mx 0.1.185

//! Lightweight local KV store for fast agent state.
//!
//! Backed by a TOML schema file and a JSON data file. All writes are atomic
//! (serialize to tmp, fsync, rename). No networking, no database.

use std::collections::{BTreeMap, HashSet};
use std::fs;
use std::io::Write as IoWrite;
use std::path::{Path, PathBuf};
use std::sync::OnceLock;

use anyhow::{Context, Result, bail};
use base_d::{DictionaryRegistry, HashAlgorithm, encode, hash};
use chrono::{DateTime, NaiveDate, Utc};
use serde::{Deserialize, Serialize};

use crate::cli::TimeRangeArgs;

/// Per-process flag so the legacy `~/.crewu/kv/` warning prints at most once,
/// even if both schema and data fall back to the legacy location.
static LEGACY_KV_WARNING_EMITTED: OnceLock<()> = OnceLock::new();

/// Cached dictionary registry for ID generation -- avoids re-allocating
/// the HashMap on every `generate_entry_id` call.
static DICT_REGISTRY: OnceLock<DictionaryRegistry> = OnceLock::new();

/// The single legacy-fallback warning copy. Lives here so the schema and data
/// resolvers cannot drift apart.
const LEGACY_KV_WARNING: &str = "note: reading kv from `~/.crewu/kv/` -- this default is moving to \
     `$MX_HOME/kv/` in a future release. Move your files or set \
     `MX_KV_SCHEMA` / `MX_KV_DATA`.";

// ---------------------------------------------------------------------------
// Path resolution
// ---------------------------------------------------------------------------

/// Pure decision: should the consolidated legacy-warning be emitted now?
///
/// Returns true exactly once across the lifetime of the supplied `gate`,
/// and only when at least one of the resolvers reported a legacy fallback.
/// Lifting this out of `from_env` keeps the dedupe logic unit-testable
/// without touching process-global stderr or env vars.
pub(crate) fn should_emit_legacy_kv_warning(
    schema_warn: bool,
    data_warn: bool,
    gate: &OnceLock<()>,
) -> bool {
    if !(schema_warn || data_warn) {
        return false;
    }
    // `set` returns Ok the first time, Err every time after.
    gate.set(()).is_ok()
}

/// Pure path-resolution helper for kv schema/data files.
///
/// Returns `(resolved_path, should_warn)`. `should_warn` is true only when
/// the legacy `~/.crewu/kv/` location is being used as a soft fallback.
///
/// Resolution order:
/// 1. Env override (with `{agent}` placeholder substitution); empty value
///    is treated as unset
/// 2. New `$MX_HOME/kv/...` location (if file exists)
/// 3. Legacy `~/.crewu/kv/...` (if file exists) -- emits warning
/// 4. Otherwise, return the new location (so error messages point at the
///    canonical place)
///
/// TODO(kv-path-migration): drop the legacy fallback after one release cycle.
pub(crate) fn resolve_kv_path_with(
    env_val: Option<&str>,
    agent: &str,
    new_default: PathBuf,
    legacy: Option<PathBuf>,
) -> (PathBuf, bool) {
    if let Some(p) = env_val
        && !p.is_empty()
    {
        return (PathBuf::from(p.replace("{agent}", agent)), false);
    }

    if new_default.exists() {
        return (new_default, false);
    }

    if let Some(legacy) = legacy
        && legacy.exists()
    {
        return (legacy, true);
    }

    (new_default, false)
}

// ---------------------------------------------------------------------------
// Exit codes
// ---------------------------------------------------------------------------

pub const EXIT_OK: i32 = 0;
pub const EXIT_KEY_NOT_FOUND: i32 = 1;
pub const EXIT_TYPE_MISMATCH: i32 = 2;
pub const EXIT_SCHEMA_MISSING: i32 = 3;
pub const EXIT_INVALID_INPUT: i32 = 4;

// ---------------------------------------------------------------------------
// Typed errors
// ---------------------------------------------------------------------------

#[derive(Debug)]
pub enum KvError {
    KeyNotFound(String),
    TypeMismatch {
        key: String,
        expected: String,
        got: String,
    },
    SchemaMissing(PathBuf),
    EntryNotFound {
        key: String,
        id: String,
    },
    AmbiguousId {
        prefix: String,
        count: usize,
    },
    DataValidation {
        message: String,
    },
    Other(anyhow::Error),
}

impl std::fmt::Display for KvError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            KvError::KeyNotFound(key) => write!(f, "Unknown key: {}", key),
            KvError::TypeMismatch { key, expected, got } => {
                write!(
                    f,
                    "Type mismatch: key '{}' is {}, not {}",
                    key, got, expected
                )
            }
            KvError::SchemaMissing(path) => {
                write!(f, "Schema file not found: {}", path.display())
            }
            KvError::EntryNotFound { key, id } => {
                write!(f, "Entry not found: ID {} in key '{}'", id, key)
            }
            KvError::AmbiguousId { prefix, count } => {
                write!(
                    f,
                    "ID prefix 'kv-{}' is ambiguous: matches {} entries, provide more characters",
                    prefix, count
                )
            }
            KvError::DataValidation { message } => write!(f, "{}", message),
            KvError::Other(e) => write!(f, "{}", e),
        }
    }
}

impl std::error::Error for KvError {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match self {
            KvError::Other(e) => Some(e.as_ref()),
            _ => None,
        }
    }
}

impl From<anyhow::Error> for KvError {
    fn from(e: anyhow::Error) -> Self {
        KvError::Other(e)
    }
}

// ---------------------------------------------------------------------------
// Schema types (parsed from TOML)
// ---------------------------------------------------------------------------

/// Top-level schema definition.
#[derive(Debug, Serialize, Deserialize)]
pub struct Schema {
    pub keys: BTreeMap<String, KeyDef>,
}

/// Definition of a single key in the schema.
#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct KeyDef {
    #[serde(rename = "type")]
    pub value_type: ValueType,

    #[serde(default)]
    pub min: Option<i64>,

    #[serde(default)]
    pub max: Option<i64>,

    #[serde(default)]
    pub default: Option<String>,

    #[serde(default)]
    pub max_entries: Option<usize>,

    #[serde(default)]
    pub description: Option<String>,

    #[serde(default)]
    pub fields: Option<Vec<String>>,

    /// Optional typed field definitions for `--data` validation.
    /// When present, `push` validates the data object against these defs.
    /// When absent, any JSON object is accepted (freeform).
    #[serde(default)]
    pub data: Option<BTreeMap<String, DataFieldDef>>,
}

/// The type of a data field in a schema's `[keys.X.data]` section.
#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq)]
#[serde(rename_all = "lowercase")]
pub enum DataFieldType {
    String,
    Number,
    Boolean,
    Array,
    Object,
}

impl std::fmt::Display for DataFieldType {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            DataFieldType::String => write!(f, "string"),
            DataFieldType::Number => write!(f, "number"),
            DataFieldType::Boolean => write!(f, "boolean"),
            DataFieldType::Array => write!(f, "array"),
            DataFieldType::Object => write!(f, "object"),
        }
    }
}

/// Definition of a single data field within a key's `[keys.X.data]` section.
#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct DataFieldDef {
    #[serde(rename = "type")]
    pub field_type: DataFieldType,

    #[serde(default)]
    pub required: bool,

    /// Parsed but not used until Part 3 (migration).
    #[serde(default)]
    pub default: Option<String>,
}

#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq)]
#[serde(rename_all = "lowercase")]
pub enum ValueType {
    Counter,
    History,
    State,
    String,
    List,
}

impl std::fmt::Display for ValueType {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            ValueType::Counter => write!(f, "counter"),
            ValueType::History => write!(f, "history"),
            ValueType::State => write!(f, "state"),
            ValueType::String => write!(f, "string"),
            ValueType::List => write!(f, "list"),
        }
    }
}

// ---------------------------------------------------------------------------
// Data types (persisted as JSON)
// ---------------------------------------------------------------------------

/// Top-level data file.
#[derive(Debug, Serialize, Deserialize, Default)]
pub struct DataFile {
    #[serde(rename = "_schema")]
    pub schema_id: String,

    #[serde(rename = "_updated")]
    pub updated: String,

    #[serde(flatten)]
    pub entries: BTreeMap<String, DataValue>,
}

/// A single stored value.
///
/// Deserialization note: we use a custom Deserialize impl to handle backward
/// compatibility for lists (old format: `items: ["a","b"]`, new format:
/// `items: [{id,value,ts}, ...]`) and for history entries that may lack `id`.
#[derive(Debug, Serialize, Clone)]
#[serde(untagged)]
pub enum DataValue {
    Counter {
        value: i64,
    },
    String {
        value: String,
    },
    History {
        entries: Vec<HistoryEntry>,
        #[serde(default, skip_serializing_if = "Option::is_none")]
        memory: Option<String>,
    },
    State {
        fields: BTreeMap<String, String>,
        #[serde(default, skip_serializing_if = "Option::is_none")]
        memory: Option<String>,
    },
    List {
        items: Vec<ListEntry>,
        #[serde(default, skip_serializing_if = "Option::is_none")]
        memory: Option<String>,
    },
}

// ---------------------------------------------------------------------------
// Backward-compatible deserialization
// ---------------------------------------------------------------------------

/// Intermediate type that can accept either old bare-string lists or new ListEntry lists.
#[derive(Deserialize)]
#[serde(untagged)]
enum RawListItem {
    Entry(ListEntry),
    Bare(String),
}

/// Mirror of DataValue for deserialization, with backward compat glue.
///
/// The `data` field on `HistoryEntry` and `ListEntry` was added after initial
/// release. Backward compatibility for files written before that field existed
/// is handled by `#[serde(default)]` on each entry struct's `data` field, so
/// `DataValueDe` itself needs no special handling for it.
#[derive(Deserialize)]
#[serde(untagged)]
enum DataValueDe {
    Counter {
        value: i64,
    },
    History {
        entries: Vec<HistoryEntry>,
        #[serde(default)]
        memory: Option<String>,
    },
    State {
        fields: BTreeMap<String, String>,
        #[serde(default)]
        memory: Option<String>,
    },
    List {
        items: Vec<RawListItem>,
        #[serde(default)]
        memory: Option<String>,
    },
    // String must be last in untagged — it's the broadest match
    String {
        value: String,
    },
}

impl<'de> Deserialize<'de> for DataValue {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: serde::Deserializer<'de>,
    {
        let raw = DataValueDe::deserialize(deserializer)?;
        Ok(match raw {
            DataValueDe::Counter { value } => DataValue::Counter { value },
            DataValueDe::String { value } => DataValue::String { value },
            DataValueDe::History { entries, memory } => {
                // Back-fill indexes for history entries loaded with index=0 (old data)
                let mut max_idx = entries.iter().map(|e| e.index).max().unwrap_or(0);
                let entries = entries
                    .into_iter()
                    .map(|mut e| {
                        if e.index == 0 {
                            max_idx += 1;
                            e.index = max_idx;
                        }
                        e
                    })
                    .collect();
                DataValue::History { entries, memory }
            }
            DataValueDe::State { fields, memory } => DataValue::State { fields, memory },
            DataValueDe::List { items, memory } => {
                let mut next_idx = 0u64;
                let entries: Vec<ListEntry> = items
                    .into_iter()
                    .map(|item| match item {
                        RawListItem::Entry(e) => {
                            if e.index >= next_idx {
                                next_idx = e.index + 1;
                            }
                            e
                        }
                        RawListItem::Bare(s) => {
                            next_idx += 1;
                            ListEntry {
                                index: next_idx,
                                id: String::new(),
                                value: s,
                                ts: String::new(),
                                data: None,
                                memory: None,
                            }
                        }
                    })
                    .collect();
                DataValue::List {
                    items: entries,
                    memory,
                }
            }
        })
    }
}

#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct HistoryEntry {
    #[serde(default, rename = "id")]
    pub index: u64,
    #[serde(default, rename = "hash")]
    pub id: String,
    pub value: String,
    pub ts: String,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub data: Option<serde_json::Value>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub memory: Option<String>,
}

#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct ListEntry {
    #[serde(rename = "id")]
    pub index: u64,
    #[serde(default, rename = "hash")]
    pub id: String,
    pub value: String,
    pub ts: String,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub data: Option<serde_json::Value>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub memory: Option<String>,
}

/// Shared field access for entry types that live in History and List values.
///
/// Both `HistoryEntry` and `ListEntry` have identical fields; this trait lets
/// shared update logic operate on either type without duplication.
pub trait UpdateableEntry {
    fn entry_index(&self) -> u64;
    fn entry_id(&self) -> &str;
    fn entry_data(&self) -> &Option<serde_json::Value>;
    fn set_value(&mut self, v: &str);
    fn set_data(&mut self, d: Option<serde_json::Value>);
}

impl UpdateableEntry for HistoryEntry {
    fn entry_index(&self) -> u64 {
        self.index
    }
    fn entry_id(&self) -> &str {
        &self.id
    }
    fn entry_data(&self) -> &Option<serde_json::Value> {
        &self.data
    }
    fn set_value(&mut self, v: &str) {
        self.value = v.to_string();
    }
    fn set_data(&mut self, d: Option<serde_json::Value>) {
        self.data = d;
    }
}

impl UpdateableEntry for ListEntry {
    fn entry_index(&self) -> u64 {
        self.index
    }
    fn entry_id(&self) -> &str {
        &self.id
    }
    fn entry_data(&self) -> &Option<serde_json::Value> {
        &self.data
    }
    fn set_value(&mut self, v: &str) {
        self.value = v.to_string();
    }
    fn set_data(&mut self, d: Option<serde_json::Value>) {
        self.data = d;
    }
}

/// Find the slice index for an entry identified by `id`, handling ambiguous
/// prefix matches.  Returns the positional index into `entries`.
fn find_entry_idx<T: UpdateableEntry>(
    entries: &[T],
    id: &IdRef,
    key: &str,
) -> Result<usize, KvError> {
    match id {
        IdRef::Index(n) => entries
            .iter()
            .position(|e| e.entry_index() == *n)
            .ok_or_else(|| KvError::EntryNotFound {
                key: key.to_string(),
                id: format!("{:?}", id),
            }),
        IdRef::Id(h) => {
            let matches: Vec<usize> = entries
                .iter()
                .enumerate()
                .filter(|(_, e)| e.entry_id().starts_with(h.as_str()))
                .map(|(i, _)| i)
                .collect();
            match matches.len() {
                0 => Err(KvError::EntryNotFound {
                    key: key.to_string(),
                    id: format!("{:?}", id),
                }),
                1 => Ok(matches[0]),
                n => Err(KvError::AmbiguousId {
                    prefix: h.clone(),
                    count: n,
                }),
            }
        }
    }
}

/// Shared update logic for History and List entries.
///
/// 1. Locate the entry by positional index.
/// 2. Compute and validate the merged data against `def`.
/// 3. Apply `new_value` and merged data in-place.
fn apply_entry_update<T: UpdateableEntry>(
    entries: &mut [T],
    id: &IdRef,
    key: &str,
    new_value: Option<&str>,
    new_data: Option<&serde_json::Value>,
    def: &KeyDef,
) -> Result<UpdateResult, KvError> {
    let idx = find_entry_idx(entries, id, key)?;

    // Compute merged data from the located entry (immutable borrow ends here).
    let merged_data = {
        let entry_data = entries[idx].entry_data();
        match (entry_data, new_data) {
            (_, None) => entry_data.clone(),
            (None, Some(patch)) => {
                if !patch.is_object() {
                    return Err(KvError::DataValidation {
                        message: format!("key '{}': --data must be a JSON object", key),
                    });
                }
                let mut obj = serde_json::Map::new();
                for (k, v) in patch.as_object().unwrap() {
                    if !v.is_null() {
                        obj.insert(k.clone(), v.clone());
                    }
                }
                if obj.is_empty() {
                    None
                } else {
                    Some(serde_json::Value::Object(obj))
                }
            }
            (Some(existing), Some(patch)) => {
                let mut merged_obj = match existing.as_object() {
                    Some(o) => o.clone(),
                    None => {
                        return Err(KvError::Other(anyhow::anyhow!(
                            "Data corruption: key '{}' entry has non-object data",
                            key
                        )));
                    }
                };
                let patch_obj = match patch.as_object() {
                    Some(p) => p,
                    None => {
                        return Err(KvError::DataValidation {
                            message: format!("key '{}': --data must be a JSON object", key),
                        });
                    }
                };
                for (k, v) in patch_obj {
                    if v.is_null() {
                        merged_obj.remove(k);
                    } else {
                        merged_obj.insert(k.clone(), v.clone());
                    }
                }
                if merged_obj.is_empty() {
                    None
                } else {
                    Some(serde_json::Value::Object(merged_obj))
                }
            }
        }
    };
    def.validate_data(key, &merged_data)?;

    // Mutate via positional index -- no unwrap needed.
    let entry = &mut entries[idx];
    if let Some(v) = new_value {
        entry.set_value(v);
    }
    let result_index = entry.entry_index();
    let result_id = entry.entry_id().to_string();
    entry.set_data(merged_data);
    Ok(UpdateResult {
        index: result_index,
        id: result_id,
    })
}

/// Remove result for the CLI layer to format output.
#[derive(Debug)]
pub struct RemoveResult {
    pub removed: Vec<String>,
}

/// Search result entry.
#[derive(Debug, Clone, serde::Serialize)]
pub struct SearchHit {
    pub index: u64,
    pub id: String,
    pub value: String,
    pub ts: String,
    #[serde(skip_serializing_if = "Option::is_none")]
    pub data: Option<serde_json::Value>,
    #[serde(skip_serializing_if = "Option::is_none")]
    pub memory: Option<String>,
}

/// Count result.
#[derive(Debug)]
pub struct CountResult {
    /// Number of entries that matched (all entries when no filter, matching entries when filtered).
    pub matched: usize,
    /// Total entries for the key. Present only when a value filter was applied.
    pub total: Option<usize>,
    pub latest_ts: Option<String>,
}

/// Result of a `migrate` operation.
#[derive(Debug)]
pub struct MigrateResult {
    pub examined: usize,
    pub modified: usize,
    pub warnings: Vec<String>,
    pub changes: Vec<MigrateChange>,
}

/// Per-entry change detail from a migration.
#[derive(Debug)]
pub struct MigrateChange {
    pub index: u64,
    pub id: String,
    pub fields_added: Vec<String>,
    pub fields_pruned: Vec<String>,
}

// ---------------------------------------------------------------------------
// Hash ID generation
// ---------------------------------------------------------------------------

/// Generate a short, stable base58 ID for a kv entry.
///
/// Input: `"{key}:{ts}:{index}"` hashed with blake3, first 4 bytes encoded as
/// base58.  Produces a ~5-6 character alphanumeric string.
pub fn generate_entry_id(key: &str, ts: &str, index: u64) -> String {
    let input = format!("{}:{}:{}", key, ts, index);
    let hash_bytes = hash(input.as_bytes(), HashAlgorithm::Blake3);
    let registry = DICT_REGISTRY
        .get_or_init(|| DictionaryRegistry::load_default().expect("base-d dictionaries"));
    let dict = registry.dictionary("base58").expect("base58 dictionary");
    encode(&hash_bytes[..4], &dict)
}

/// Result of a push operation, carrying both the index and the stable ID.
#[derive(Debug, Clone)]
pub struct PushResult {
    pub index: u64,
    pub id: String,
}

/// Result of an `update_entry` operation.
#[derive(Debug)]
pub struct UpdateResult {
    pub index: u64,
    pub id: String,
}

/// Reference to a kv entry by either its numeric index or its stable ID.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum IdRef {
    Index(u64),
    Id(String),
}

// ---------------------------------------------------------------------------
// Data field validation
// ---------------------------------------------------------------------------

fn json_type_name(v: &serde_json::Value) -> &'static str {
    match v {
        serde_json::Value::Null => "null",
        serde_json::Value::Bool(_) => "boolean",
        serde_json::Value::Number(_) => "number",
        serde_json::Value::String(_) => "string",
        serde_json::Value::Array(_) => "array",
        serde_json::Value::Object(_) => "object",
    }
}

/// Parse a raw default-value string into a typed `serde_json::Value`.
///
/// Used by `migrate` to fill missing fields from schema defaults.
pub(crate) fn parse_default_value(
    raw: &str,
    field_type: DataFieldType,
) -> Result<serde_json::Value, KvError> {
    match field_type {
        DataFieldType::String => Ok(serde_json::Value::String(raw.to_string())),
        DataFieldType::Number => {
            let n: serde_json::Number = raw.parse().map_err(|_| KvError::DataValidation {
                message: format!("cannot parse default '{}' as number", raw),
            })?;
            Ok(serde_json::Value::Number(n))
        }
        DataFieldType::Boolean => {
            let b: bool = raw.parse().map_err(|_| KvError::DataValidation {
                message: format!(
                    "cannot parse default '{}' as boolean (expected \"true\" or \"false\")",
                    raw
                ),
            })?;
            Ok(serde_json::Value::Bool(b))
        }
        DataFieldType::Array | DataFieldType::Object => {
            let v: serde_json::Value =
                serde_json::from_str(raw).map_err(|e| KvError::DataValidation {
                    message: format!("cannot parse default '{}' as {}: {}", raw, field_type, e),
                })?;
            let type_ok = match field_type {
                DataFieldType::Array => v.is_array(),
                DataFieldType::Object => v.is_object(),
                _ => unreachable!(),
            };
            if !type_ok {
                return Err(KvError::DataValidation {
                    message: format!(
                        "default '{}' parsed as {} but expected {}",
                        raw,
                        json_type_name(&v),
                        field_type
                    ),
                });
            }
            Ok(v)
        }
    }
}

impl KeyDef {
    /// Validate `--data` against this key's field definitions.
    ///
    /// When `data` is `Some`, validates the object against `self.data` defs.
    /// When `data` is `None` and required fields exist, returns an error.
    /// When `self.data` is `None` (freeform), always passes.
    pub fn validate_data(
        &self,
        key: &str,
        data: &Option<serde_json::Value>,
    ) -> Result<(), KvError> {
        let field_defs = match self.data {
            Some(ref defs) => defs,
            None => return Ok(()),
        };

        let obj = match data {
            Some(val) => match val.as_object() {
                Some(o) => o,
                None => {
                    return Err(KvError::DataValidation {
                        message: format!("key '{}': --data must be a JSON object", key),
                    });
                }
            },
            None => {
                let required: Vec<&str> = field_defs
                    .iter()
                    .filter(|(_, d)| d.required)
                    .map(|(n, _)| n.as_str())
                    .collect();
                if !required.is_empty() {
                    return Err(KvError::DataValidation {
                        message: format!(
                            "key '{}': --data is required (schema has required fields: {})",
                            key,
                            required.join(", "),
                        ),
                    });
                }
                return Ok(());
            }
        };

        // Null values are treated as absent — they don't trigger undeclared-field
        // errors, don't satisfy required-field checks, and skip type checking.

        // 1. Reject undeclared fields (ignoring nulls)
        let extra: Vec<&str> = obj
            .iter()
            .filter(|(_, v)| !v.is_null())
            .filter(|(k, _)| !field_defs.contains_key(k.as_str()))
            .map(|(k, _)| k.as_str())
            .collect();
        if !extra.is_empty() {
            let declared: Vec<&str> = field_defs.keys().map(|s| s.as_str()).collect();
            return Err(KvError::DataValidation {
                message: format!(
                    "key '{}': undeclared data fields: {}; declared fields: {}",
                    key,
                    extra.join(", "),
                    declared.join(", "),
                ),
            });
        }

        // 2. Required fields present (null counts as absent)
        let missing: Vec<&str> = field_defs
            .iter()
            .filter(|(_, d)| d.required)
            .filter(|(name, _)| obj.get(name.as_str()).is_none_or(|v| v.is_null()))
            .map(|(name, _)| name.as_str())
            .collect();
        if !missing.is_empty() {
            return Err(KvError::DataValidation {
                message: format!(
                    "key '{}': missing required data fields: {}",
                    key,
                    missing.join(", "),
                ),
            });
        }

        // 3. Type checking — batch all mismatches (nulls skipped)
        let bad: Vec<std::string::String> = obj
            .iter()
            .filter(|(_, v)| !v.is_null())
            .filter_map(|(name, value)| {
                let def = field_defs.get(name.as_str())?;
                let ok = match def.field_type {
                    DataFieldType::String => value.is_string(),
                    DataFieldType::Number => value.is_number(),
                    DataFieldType::Boolean => value.is_boolean(),
                    DataFieldType::Array => value.is_array(),
                    DataFieldType::Object => value.is_object(),
                };
                if ok {
                    None
                } else {
                    Some(format!(
                        "'{}' must be {}, got {}",
                        name,
                        def.field_type,
                        json_type_name(value),
                    ))
                }
            })
            .collect();
        if !bad.is_empty() {
            return Err(KvError::DataValidation {
                message: format!("key '{}': type errors: {}", key, bad.join("; ")),
            });
        }

        Ok(())
    }
}

// ---------------------------------------------------------------------------
// KV Store
// ---------------------------------------------------------------------------

pub struct KvStore {
    pub schema: Schema,
    pub data: DataFile,
    pub data_path: PathBuf,
    pub schema_path: PathBuf,
}

impl KvStore {
    /// Load schema and data from the given paths. Creates data file with defaults
    /// if it doesn't exist.
    pub fn load(schema_path: &Path, data_path: &Path) -> Result<Self> {
        let schema_str = fs::read_to_string(schema_path)
            .with_context(|| format!("Failed to read schema: {}", schema_path.display()))?;
        let schema: Schema = toml::from_str(&schema_str)
            .with_context(|| format!("Failed to parse schema: {}", schema_path.display()))?;

        let mut data = if data_path.exists() {
            let data_str = fs::read_to_string(data_path)
                .with_context(|| format!("Failed to read data: {}", data_path.display()))?;
            serde_json::from_str(&data_str)
                .with_context(|| format!("Failed to parse data: {}", data_path.display()))?
        } else {
            DataFile::default()
        };

        // Back-fill IDs for entries loaded without one (pre-ID data).
        let mut needs_save = false;
        for (key, value) in &mut data.entries {
            match value {
                DataValue::History { entries, .. } => {
                    for e in entries.iter_mut() {
                        if e.id.is_empty() {
                            e.id = generate_entry_id(key, &e.ts, e.index);
                            needs_save = true;
                        }
                    }
                }
                DataValue::List { items, .. } => {
                    for e in items.iter_mut() {
                        if e.id.is_empty() {
                            e.id = generate_entry_id(key, &e.ts, e.index);
                            needs_save = true;
                        }
                    }
                }
                _ => {}
            }
        }

        let mut store = KvStore {
            schema,
            data,
            data_path: data_path.to_path_buf(),
            schema_path: schema_path.to_path_buf(),
        };

        if needs_save {
            store.save()?;
        }

        Ok(store)
    }

    /// Load from environment variables. Resolves {agent} placeholder.
    ///
    /// Resolution order for schema:
    /// 1. `MX_KV_SCHEMA` env var (with `{agent}` placeholder substitution)
    /// 2. `$MX_HOME/kv/schema/{agent}.toml` (new default)
    /// 3. Soft fallback: `~/.crewu/kv/{agent}.schema.toml` (legacy, with stderr note)
    ///
    /// Same shape for data via `MX_KV_DATA` and `~/.crewu/kv/{agent}.data.json`.
    ///
    /// TODO(kv-path-migration): remove the `~/.crewu/kv/` fallback after one
    /// release cycle.
    pub fn from_env() -> Result<Self> {
        let agent = std::env::var("MX_CURRENT_AGENT")
            .with_context(|| "MX_CURRENT_AGENT environment variable is required")?;

        let (schema_path, schema_warn) = Self::resolve_schema_path(&agent);
        let (data_path, data_warn) = Self::resolve_data_path(&agent);

        // Suppress the legacy warning when resolved files live under MX_HOME --
        // the user declared this directory as home, so files there with legacy
        // naming aren't really "legacy."
        let under_mx_home = schema_path.starts_with(crate::paths::mx_home())
            && data_path.starts_with(crate::paths::mx_home());
        if !under_mx_home
            && should_emit_legacy_kv_warning(schema_warn, data_warn, &LEGACY_KV_WARNING_EMITTED)
        {
            eprintln!("{}", LEGACY_KV_WARNING);
        }

        let mut store = Self::load(&schema_path, &data_path)?;

        // Populate _schema field from agent name if empty (SHOULD-FIX 5)
        if store.data.schema_id.is_empty() {
            store.data.schema_id = agent.clone();
        }

        Ok(store)
    }

    /// Resolve the schema path for an agent, delegating to the testable
    /// `resolve_kv_path_with` seam. Returns `(path, should_warn)` where
    /// `should_warn` is true only if the legacy `~/.crewu/kv/` location was
    /// used as a soft fallback.
    fn resolve_schema_path(agent: &str) -> (PathBuf, bool) {
        resolve_kv_path_with(
            std::env::var("MX_KV_SCHEMA").ok().as_deref(),
            agent,
            crate::paths::kv_schema_path(agent),
            crate::paths::legacy_crewu_kv_schema_path(agent),
        )
    }

    /// Resolve the data path for an agent, delegating to the testable
    /// `resolve_kv_path_with` seam. Returns `(path, should_warn)` where
    /// `should_warn` is true only if the legacy `~/.crewu/kv/` location was
    /// used as a soft fallback.
    fn resolve_data_path(agent: &str) -> (PathBuf, bool) {
        resolve_kv_path_with(
            std::env::var("MX_KV_DATA").ok().as_deref(),
            agent,
            crate::paths::kv_data_path(agent),
            crate::paths::legacy_crewu_kv_data_path(agent),
        )
    }

    /// Atomic write: serialize to tmp, fsync, rename.
    pub fn save(&mut self) -> Result<()> {
        self.data.updated = Utc::now().to_rfc3339();

        let json = serde_json::to_string_pretty(&self.data).context("Failed to serialize data")?;

        // Ensure parent directory exists
        if let Some(parent) = self.data_path.parent() {
            fs::create_dir_all(parent)
                .with_context(|| format!("Failed to create directory: {}", parent.display()))?;
        }

        let tmp_path = self
            .data_path
            .with_extension(format!("tmp.{}", std::process::id()));

        {
            let mut f = fs::File::create(&tmp_path)
                .with_context(|| format!("Failed to create temp file: {}", tmp_path.display()))?;
            f.write_all(json.as_bytes())?;
            f.sync_all()?;
        }

        fs::rename(&tmp_path, &self.data_path).with_context(|| {
            format!(
                "Failed to rename {} -> {}",
                tmp_path.display(),
                self.data_path.display()
            )
        })?;

        Ok(())
    }

    // -----------------------------------------------------------------------
    // Schema helpers
    // -----------------------------------------------------------------------

    fn key_def(&self, key: &str) -> Result<&KeyDef, KvError> {
        self.schema
            .keys
            .get(key)
            .ok_or_else(|| KvError::KeyNotFound(key.to_string()))
    }

    fn assert_type(&self, key: &str, expected: ValueType) -> Result<&KeyDef, KvError> {
        let def = self.key_def(key)?;
        if def.value_type != expected {
            return Err(KvError::TypeMismatch {
                key: key.to_string(),
                expected: expected.to_string(),
                got: def.value_type.to_string(),
            });
        }
        Ok(def)
    }

    /// Validate a key name for schema insertion.
    ///
    /// Accepts alphanumeric characters, underscores, and hyphens.
    /// Rejects names containing dots (TOML quoting issues), empty names,
    /// and names with other special characters.
    fn validate_key_name(key: &str) -> Result<(), KvError> {
        if key.is_empty() {
            return Err(KvError::DataValidation {
                message: "key name cannot be empty".to_string(),
            });
        }
        if key.len() > 128 {
            return Err(KvError::DataValidation {
                message: format!("key name too long ({} chars, max 128)", key.len()),
            });
        }
        if key.contains('.') {
            return Err(KvError::DataValidation {
                message: format!(
                    "key name '{}' cannot contain dots -- they require TOML quoting and create confusion",
                    key
                ),
            });
        }
        if !key
            .chars()
            .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
        {
            return Err(KvError::DataValidation {
                message: format!(
                    "key name '{}' contains invalid characters -- only alphanumeric, underscores, and hyphens are allowed",
                    key
                ),
            });
        }
        Ok(())
    }

    /// Add a new key to the schema file and reload the in-memory schema.
    ///
    /// Appends a `[keys.<name>]` block to the TOML file (preserving existing
    /// content exactly) and re-parses the file to update `self.schema`.
    ///
    /// Only `history` and `list` types are accepted -- those are the types
    /// that support `push`.
    pub fn add_key_to_schema(
        &mut self,
        key: &str,
        value_type: &str,
        max_entries: Option<usize>,
    ) -> Result<(), KvError> {
        // Validate key name
        Self::validate_key_name(key)?;

        // Double-check: don't overwrite existing keys
        if self.schema.keys.contains_key(key) {
            return Ok(());
        }

        // Build the TOML block to append
        let mut block = format!("\n[keys.{}]\ntype = \"{}\"\n", key, value_type);
        if let Some(max) = max_entries {
            block.push_str(&format!("max_entries = {}\n", max));
        }

        // Append to the schema file
        let mut f = fs::OpenOptions::new()
            .append(true)
            .open(&self.schema_path)
            .map_err(|e| {
                KvError::Other(anyhow::anyhow!(
                    "failed to open schema file for append: {}: {}",
                    self.schema_path.display(),
                    e
                ))
            })?;
        f.write_all(block.as_bytes()).map_err(|e| {
            KvError::Other(anyhow::anyhow!(
                "failed to write to schema file: {}: {}",
                self.schema_path.display(),
                e
            ))
        })?;
        drop(f);

        // Re-read and re-parse the schema to update in-memory state
        let schema_str = fs::read_to_string(&self.schema_path).map_err(|e| {
            KvError::Other(anyhow::anyhow!(
                "failed to re-read schema file after append: {}: {}",
                self.schema_path.display(),
                e
            ))
        })?;
        let schema: Schema = toml::from_str(&schema_str).map_err(|e| {
            KvError::Other(anyhow::anyhow!(
                "failed to re-parse schema file after append: {}: {}",
                self.schema_path.display(),
                e
            ))
        })?;
        self.schema = schema;

        Ok(())
    }

    /// Atomic write of the schema file: serialize to TOML, write to tmp, fsync, rename.
    ///
    /// This is the schema counterpart of `save()` (which persists only the data
    /// file). Schema serialization via `toml = "0.8"` drops comments and custom
    /// formatting -- this matches the existing pattern established by
    /// `add_key_to_schema` which re-parses after appending.
    ///
    /// Takes `&self` rather than `&mut self` because it only serializes the
    /// current schema to disk -- unlike `save()`, it does not update timestamps
    /// or other mutable bookkeeping.
    fn save_schema(&self) -> Result<()> {
        let toml_str =
            toml::to_string_pretty(&self.schema).context("Failed to serialize schema to TOML")?;

        // Ensure parent directory exists
        if let Some(parent) = self.schema_path.parent() {
            fs::create_dir_all(parent)
                .with_context(|| format!("Failed to create directory: {}", parent.display()))?;
        }

        let tmp_path = self
            .schema_path
            .with_extension(format!("tmp.{}", std::process::id()));

        {
            let mut f = fs::File::create(&tmp_path)
                .with_context(|| format!("Failed to create temp file: {}", tmp_path.display()))?;
            f.write_all(toml_str.as_bytes())?;
            f.sync_all()?;
        }

        fs::rename(&tmp_path, &self.schema_path).with_context(|| {
            format!(
                "Failed to rename {} -> {}",
                tmp_path.display(),
                self.schema_path.display()
            )
        })?;

        Ok(())
    }

    /// Rename a key, preserving all entries and data.
    ///
    /// Mutates in-memory state fully before writing either file. Data is
    /// persisted first (higher-value file), then schema. If the data write
    /// fails, in-memory mutations are rolled back so the `KvStore` stays
    /// clean. If data succeeds but the schema write fails, the on-disk data
    /// file already contains the renamed key while the schema still
    /// references the old name -- a narrow consistency gap that callers
    /// should be aware of. In practice both writes target the same
    /// directory, so a failure between them is unlikely.
    ///
    /// Entry IDs are stable -- they were hashed from the original key name at
    /// creation time and are never regenerated.
    pub fn rename_key(&mut self, old_key: &str, new_key: &str) -> Result<(), KvError> {
        // Validate new key name
        Self::validate_key_name(new_key)?;

        // old_key must exist in schema
        if !self.schema.keys.contains_key(old_key) {
            return Err(KvError::KeyNotFound(old_key.to_string()));
        }

        // new_key must not already exist
        if self.schema.keys.contains_key(new_key) {
            return Err(KvError::DataValidation {
                message: format!("Key already exists: {}", new_key),
            });
        }

        // Move schema entry: remove old, insert new
        let key_def = self.schema.keys.remove(old_key).expect("checked above");
        self.schema.keys.insert(new_key.to_string(), key_def);

        // Move data entry (if it exists -- key might have no data yet)
        let had_data = self.data.entries.contains_key(old_key);
        if let Some(data_value) = self.data.entries.remove(old_key) {
            self.data.entries.insert(new_key.to_string(), data_value);
        }

        // Persist data first (higher-value file), then schema.
        if let Err(e) = self.save() {
            // Rollback in-memory mutations so the KvStore stays clean.
            let key_def = self.schema.keys.remove(new_key).expect("just inserted");
            self.schema.keys.insert(old_key.to_string(), key_def);
            if had_data && let Some(data_value) = self.data.entries.remove(new_key) {
                self.data.entries.insert(old_key.to_string(), data_value);
            }
            return Err(KvError::Other(e));
        }

        // Schema write -- if this fails, data is already persisted with the
        // new key name. See doc comment for the consistency trade-off.
        self.save_schema().map_err(KvError::Other)?;

        Ok(())
    }

    /// Get the default DataValue for a key based on its schema definition.
    fn default_value(def: &KeyDef) -> DataValue {
        match def.value_type {
            ValueType::Counter => {
                let default_val: i64 = def
                    .default
                    .as_ref()
                    .and_then(|s| s.parse().ok())
                    .unwrap_or(0);
                DataValue::Counter { value: default_val }
            }
            ValueType::String => DataValue::String {
                value: def.default.clone().unwrap_or_default(),
            },
            ValueType::History => DataValue::History {
                entries: Vec::new(),
                memory: None,
            },
            ValueType::State => {
                let fields = def
                    .fields
                    .as_ref()
                    .map(|fs| fs.iter().map(|f| (f.clone(), String::new())).collect())
                    .unwrap_or_default();
                DataValue::State {
                    fields,
                    memory: None,
                }
            }
            ValueType::List => DataValue::List {
                items: Vec::new(),
                memory: None,
            },
        }
    }

    // -----------------------------------------------------------------------
    // Operations
    // -----------------------------------------------------------------------

    /// Get the current value for a key.
    pub fn get(&self, key: &str) -> Result<&DataValue, KvError> {
        let def = self.key_def(key)?;
        match self.data.entries.get(key) {
            Some(v) => Ok(v),
            None => Err(KvError::KeyNotFound(format!(
                "{} (has no data yet, type: {})",
                key, def.value_type
            ))),
        }
    }

    /// Set a string or counter value, or set a field on a state type.
    pub fn set(&mut self, key: &str, value: &str, field: Option<&str>) -> Result<(), KvError> {
        let def = self.key_def(key)?.clone();

        match def.value_type {
            ValueType::String => {
                self.data.entries.insert(
                    key.to_string(),
                    DataValue::String {
                        value: value.to_string(),
                    },
                );
            }
            ValueType::Counter => {
                let v: i64 = value.parse().map_err(|_| {
                    KvError::Other(anyhow::anyhow!("Invalid counter value: {}", value))
                })?;
                let v = Self::clamp(v, def.min, def.max);
                self.data
                    .entries
                    .insert(key.to_string(), DataValue::Counter { value: v });
            }
            ValueType::State => {
                let field_name = field.ok_or_else(|| {
                    KvError::Other(anyhow::anyhow!(
                        "State type requires field name: mx kv set {} <field> <value>",
                        key
                    ))
                })?;

                // Validate field name against schema
                if let Some(ref schema_fields) = def.fields
                    && !schema_fields.contains(&field_name.to_string())
                {
                    return Err(KvError::Other(anyhow::anyhow!(
                        "Unknown field '{}' for key '{}'. Valid fields: {}",
                        field_name,
                        key,
                        schema_fields.join(", ")
                    )));
                }

                let entry = self
                    .data
                    .entries
                    .entry(key.to_string())
                    .or_insert_with(|| Self::default_value(&def));

                match entry {
                    DataValue::State { fields, .. } => {
                        fields.insert(field_name.to_string(), value.to_string());
                    }
                    _ => {
                        return Err(KvError::Other(anyhow::anyhow!(
                            "Data corruption: key '{}' has wrong runtime type",
                            key
                        )));
                    }
                }
            }
            _ => {
                return Err(KvError::TypeMismatch {
                    key: key.to_string(),
                    expected: "string, counter, or state".to_string(),
                    got: def.value_type.to_string(),
                });
            }
        }

        Ok(())
    }

    /// Set multiple fields on a state-type entry in a single validate-all-then-write-all operation.
    /// Persistence is atomic (tmp + fsync + rename).
    ///
    /// - Validates the key is `ValueType::State`
    /// - Validates ALL field names against the schema before any writes
    /// - Rejects empty `fields` vec
    /// - Rejects duplicate field names in the batch
    /// - Reports all validation errors, not just the first
    /// - Preserves existing fields not mentioned in the batch (partial update)
    pub fn set_state_batch(
        &mut self,
        key: &str,
        fields: &[(String, String)],
    ) -> Result<(), KvError> {
        if fields.is_empty() {
            return Err(KvError::DataValidation {
                message: "batch set requires at least one field".to_string(),
            });
        }

        let def = self.key_def(key)?.clone();

        if def.value_type != ValueType::State {
            return Err(KvError::TypeMismatch {
                key: key.to_string(),
                expected: "state".to_string(),
                got: def.value_type.to_string(),
            });
        }

        // Check for duplicate field names in the batch
        let mut seen = HashSet::new();
        let dupes: Vec<&str> = fields
            .iter()
            .filter(|(name, _)| !seen.insert(name.as_str()))
            .map(|(name, _)| name.as_str())
            .collect();
        if !dupes.is_empty() {
            return Err(KvError::DataValidation {
                message: format!("duplicate field names in batch: {}", dupes.join(", ")),
            });
        }

        // Validate all field names against schema (batch all errors)
        if let Some(ref schema_fields) = def.fields {
            let errors: Vec<String> = fields
                .iter()
                .filter(|(name, _)| !schema_fields.contains(name))
                .map(|(name, _)| {
                    format!(
                        "unknown field '{}' (valid: {})",
                        name,
                        schema_fields.join(", ")
                    )
                })
                .collect();
            if !errors.is_empty() {
                return Err(KvError::DataValidation {
                    message: errors.join("; "),
                });
            }
        }

        // All validation passed -- apply the batch
        let entry = self
            .data
            .entries
            .entry(key.to_string())
            .or_insert_with(|| Self::default_value(&def));

        match entry {
            DataValue::State {
                fields: state_fields,
                ..
            } => {
                for (name, value) in fields {
                    state_fields.insert(name.clone(), value.clone());
                }
            }
            _ => {
                return Err(KvError::Other(anyhow::anyhow!(
                    "Data corruption: key '{}' has wrong runtime type",
                    key
                )));
            }
        }

        Ok(())
    }

    /// Set a state-type entry's fields from positional values (tensor-style).
    ///
    /// Maps each value to the corresponding schema field by position.
    /// The number of values must match the number of schema fields exactly.
    /// Delegates to `set_state_batch` after building the field pairs.
    pub fn set_tensor_batch(&mut self, key: &str, values: &[String]) -> Result<(), KvError> {
        let def = self.key_def(key)?.clone();

        let schema_fields = match def.fields {
            Some(ref f) => f,
            None => {
                return Err(KvError::DataValidation {
                    message: format!(
                        "key '{}' is state type but has no fields defined; \
                         positional set requires a fields schema",
                        key
                    ),
                });
            }
        };

        if values.len() != schema_fields.len() {
            return Err(KvError::DataValidation {
                message: format!(
                    "expected {} values ({}), got {}",
                    schema_fields.len(),
                    schema_fields.join(", "),
                    values.len()
                ),
            });
        }

        let pairs: Vec<(String, String)> = schema_fields
            .iter()
            .zip(values.iter())
            .map(|(f, v)| (f.clone(), v.clone()))
            .collect();

        self.set_state_batch(key, &pairs)
    }

    /// Increment a counter. Clamps to min/max, never errors on bounds.
    pub fn inc(&mut self, key: &str, by: i64) -> Result<i64, KvError> {
        let def = self.assert_type(key, ValueType::Counter)?.clone();

        let entry = self
            .data
            .entries
            .entry(key.to_string())
            .or_insert_with(|| Self::default_value(&def));

        match entry {
            DataValue::Counter { value } => {
                *value = Self::clamp(value.saturating_add(by), def.min, def.max);
                Ok(*value)
            }
            _ => Err(KvError::Other(anyhow::anyhow!(
                "Data corruption: key '{}' has wrong runtime type",
                key
            ))),
        }
    }

    /// Decrement a counter. Clamps to min/max, never errors on bounds.
    pub fn dec(&mut self, key: &str, by: i64) -> Result<i64, KvError> {
        let def = self.assert_type(key, ValueType::Counter)?.clone();

        let entry = self
            .data
            .entries
            .entry(key.to_string())
            .or_insert_with(|| Self::default_value(&def));

        match entry {
            DataValue::Counter { value } => {
                *value = Self::clamp(value.saturating_sub(by), def.min, def.max);
                Ok(*value)
            }
            _ => Err(KvError::Other(anyhow::anyhow!(
                "Data corruption: key '{}' has wrong runtime type",
                key
            ))),
        }
    }

    /// Push a value onto a history (with auto-timestamp) or list.
    pub fn push(
        &mut self,
        key: &str,
        value: &str,
        data: Option<serde_json::Value>,
        memory: Option<String>,
    ) -> Result<PushResult, KvError> {
        self.push_with_ts(key, value, Utc::now(), data, memory)
    }

    /// Push with an explicit timestamp (used by tests).
    pub fn push_with_ts(
        &mut self,
        key: &str,
        value: &str,
        ts: DateTime<Utc>,
        data: Option<serde_json::Value>,
        memory: Option<String>,
    ) -> Result<PushResult, KvError> {
        let def = self.key_def(key)?.clone();

        def.validate_data(key, &data)?;

        let ts_str = ts.to_rfc3339();

        match def.value_type {
            ValueType::History => {
                let entry = self
                    .data
                    .entries
                    .entry(key.to_string())
                    .or_insert_with(|| Self::default_value(&def));

                match entry {
                    DataValue::History { entries, .. } => {
                        let next_idx = entries.iter().map(|e| e.index).max().unwrap_or(0) + 1;
                        let id = generate_entry_id(key, &ts_str, next_idx);
                        entries.insert(
                            0,
                            HistoryEntry {
                                index: next_idx,
                                id: id.clone(),
                                value: value.to_string(),
                                ts: ts_str,
                                data,
                                memory,
                            },
                        );
                        // Drop oldest at max_entries
                        if let Some(max) = def.max_entries {
                            entries.truncate(max);
                        }
                        Ok(PushResult {
                            index: next_idx,
                            id,
                        })
                    }
                    _ => Err(KvError::Other(anyhow::anyhow!(
                        "Data corruption: key '{}' has wrong runtime type",
                        key
                    ))),
                }
            }
            ValueType::List => {
                let entry = self
                    .data
                    .entries
                    .entry(key.to_string())
                    .or_insert_with(|| Self::default_value(&def));

                match entry {
                    DataValue::List { items, .. } => {
                        let next_idx = items.iter().map(|e| e.index).max().unwrap_or(0) + 1;
                        let id = generate_entry_id(key, &ts_str, next_idx);
                        items.push(ListEntry {
                            index: next_idx,
                            id: id.clone(),
                            value: value.to_string(),
                            ts: ts_str,
                            data,
                            memory,
                        });
                        // Drop oldest at max_entries — single drain instead of O(n^2) remove loop
                        if let Some(max) = def.max_entries
                            && items.len() > max
                        {
                            items.drain(0..items.len() - max);
                        }
                        Ok(PushResult {
                            index: next_idx,
                            id,
                        })
                    }
                    _ => Err(KvError::Other(anyhow::anyhow!(
                        "Data corruption: key '{}' has wrong runtime type",
                        key
                    ))),
                }
            }
            _ => Err(KvError::TypeMismatch {
                key: key.to_string(),
                expected: "history or list".to_string(),
                got: def.value_type.to_string(),
            }),
        }
    }

    /// Migrate existing entries to match current schema data definitions.
    ///
    /// For each entry in a history or list key:
    /// 1. Fill missing required fields that have defaults
    /// 2. Prune undeclared fields (when `prune` is true)
    /// 3. Warn on type mismatches (no automatic coercion)
    /// 4. Create data blobs from defaults for entries with `data: None`
    ///
    /// When `dry_run` is true, reports what would change without mutating data.
    pub fn migrate(
        &mut self,
        key: &str,
        prune: bool,
        dry_run: bool,
    ) -> Result<MigrateResult, KvError> {
        let def = self.key_def(key)?.clone();

        // Reject non-entry types
        match def.value_type {
            ValueType::History | ValueType::List => {}
            other => {
                return Err(KvError::DataValidation {
                    message: format!(
                        "key '{}' is type '{}' -- migrate only works on history or list keys",
                        key, other
                    ),
                });
            }
        }

        // Check for data field definitions
        let field_defs = match def.data {
            Some(ref defs) => defs,
            None => {
                return Ok(MigrateResult {
                    examined: 0,
                    modified: 0,
                    warnings: vec![format!(
                        "key '{}': no [data] section in schema -- nothing to migrate",
                        key
                    )],
                    changes: vec![],
                });
            }
        };

        // Collect the data blobs we need to iterate over. We work on a vec of
        // (index, id, &mut Option<serde_json::Value>) tuples.
        let entry = match self.data.entries.get_mut(key) {
            Some(v) => v,
            None => {
                return Ok(MigrateResult {
                    examined: 0,
                    modified: 0,
                    warnings: vec![],
                    changes: vec![],
                });
            }
        };

        // Build a list of mutable references to entry data blobs
        let entry_refs: Vec<(u64, String, &mut Option<serde_json::Value>)> = match entry {
            DataValue::History { entries, .. } => entries
                .iter_mut()
                .map(|e| (e.index, e.id.clone(), &mut e.data))
                .collect(),
            DataValue::List { items, .. } => items
                .iter_mut()
                .map(|e| (e.index, e.id.clone(), &mut e.data))
                .collect(),
            _ => {
                return Err(KvError::Other(anyhow::anyhow!(
                    "Data corruption: key '{}' has wrong runtime type",
                    key
                )));
            }
        };

        let mut result = MigrateResult {
            examined: entry_refs.len(),
            modified: 0,
            warnings: Vec::new(),
            changes: Vec::new(),
        };

        for (index, id, data_slot) in entry_refs {
            let mut fields_added = Vec::new();
            let mut fields_pruned = Vec::new();

            match data_slot {
                Some(val) => {
                    let obj = match val.as_object_mut() {
                        Some(o) => o,
                        None => {
                            result.warnings.push(format!(
                                "entry kv-{} ({}): data is not a JSON object, skipping",
                                id, index
                            ));
                            continue;
                        }
                    };

                    // 1. Fill missing fields that have defaults; warn on required without default
                    for (field_name, field_def) in field_defs {
                        if !obj.contains_key(field_name) {
                            if let Some(ref raw_default) = field_def.default {
                                match parse_default_value(raw_default, field_def.field_type) {
                                    Ok(default_val) => {
                                        if !dry_run {
                                            obj.insert(field_name.clone(), default_val);
                                        }
                                        fields_added.push(field_name.clone());
                                    }
                                    Err(e) => {
                                        result.warnings.push(format!(
                                            "entry kv-{} ({}): failed to parse default for '{}': {}",
                                            id, index, field_name, e
                                        ));
                                    }
                                }
                            } else if field_def.required {
                                result.warnings.push(format!(
                                    "entry kv-{} ({}): required field '{}' is missing and has no default",
                                    id, index, field_name
                                ));
                            }
                        } else {
                            // Field exists -- check type
                            let existing = &obj[field_name];
                            let type_ok = match field_def.field_type {
                                DataFieldType::String => existing.is_string(),
                                DataFieldType::Number => existing.is_number(),
                                DataFieldType::Boolean => existing.is_boolean(),
                                DataFieldType::Array => existing.is_array(),
                                DataFieldType::Object => existing.is_object(),
                            };
                            // null is accepted for any field type (treated as absent on read)
                            if !type_ok && !existing.is_null() {
                                result.warnings.push(format!(
                                    "entry kv-{} ({}): field '{}' has type {} but schema expects {}",
                                    id, index, field_name, json_type_name(existing), field_def.field_type
                                ));
                            }
                        }
                    }

                    // 2. Prune undeclared fields
                    if prune {
                        let undeclared: Vec<String> = obj
                            .keys()
                            .filter(|k| !field_defs.contains_key(k.as_str()))
                            .cloned()
                            .collect();
                        for field_name in undeclared {
                            if !dry_run {
                                obj.remove(&field_name);
                            }
                            fields_pruned.push(field_name);
                        }
                    }
                }
                None => {
                    // Entry has no data blob -- create one from defaults if possible
                    let mut new_obj = serde_json::Map::new();
                    let mut created_any = false;

                    for (field_name, field_def) in field_defs {
                        if let Some(ref raw_default) = field_def.default {
                            match parse_default_value(raw_default, field_def.field_type) {
                                Ok(default_val) => {
                                    new_obj.insert(field_name.clone(), default_val);
                                    fields_added.push(field_name.clone());
                                    created_any = true;
                                }
                                Err(e) => {
                                    result.warnings.push(format!(
                                        "entry kv-{} ({}): failed to parse default for '{}': {}",
                                        id, index, field_name, e
                                    ));
                                }
                            }
                        } else if field_def.required {
                            result.warnings.push(format!(
                                "entry kv-{} ({}): required field '{}' is missing and has no default",
                                id, index, field_name
                            ));
                        }
                    }

                    if created_any && !dry_run {
                        *data_slot = Some(serde_json::Value::Object(new_obj));
                    }
                }
            }

            if !fields_added.is_empty() || !fields_pruned.is_empty() {
                result.modified += 1;
                result.changes.push(MigrateChange {
                    index,
                    id,
                    fields_added,
                    fields_pruned,
                });
            }
        }

        Ok(result)
    }

    /// Pop the last item from a list. History is append-only.
    pub fn pop(&mut self, key: &str) -> Result<Option<ListEntry>, KvError> {
        self.assert_type(key, ValueType::List)?;

        match self.data.entries.get_mut(key) {
            Some(DataValue::List { items, .. }) => Ok(items.pop()),
            Some(_) => Err(KvError::Other(anyhow::anyhow!(
                "Data corruption: key '{}' has wrong runtime type",
                key
            ))),
            None => Ok(None),
        }
    }

    /// Get the last N entries from a history or list as `SearchHit`s.
    ///
    /// When `range` is provided, entries are filtered by timestamp first, then
    /// the `count` limit is applied to the filtered set.
    /// When `where_clauses` is non-empty, entries must match all clauses.
    pub fn last(
        &self,
        key: &str,
        count: usize,
        range: Option<&TimeRange>,
        where_clauses: &[(String, String)],
    ) -> Result<Vec<SearchHit>, KvError> {
        let def = self.key_def(key)?;

        match def.value_type {
            ValueType::History => match self.data.entries.get(key) {
                Some(DataValue::History { entries, .. }) => {
                    // History stores newest-first; reverse so filtered vec is
                    // chronological (oldest-first), matching the List branch.
                    let filtered: Vec<_> = entries
                        .iter()
                        .rev()
                        .filter(|e| {
                            range.is_none_or(|r| ts_in_range(&e.ts, r))
                                && where_matches(&e.data, where_clauses)
                        })
                        .collect();
                    let start = filtered.len().saturating_sub(count);
                    Ok(filtered[start..]
                        .iter()
                        .map(|e| SearchHit {
                            index: e.index,
                            id: e.id.clone(),
                            value: e.value.clone(),
                            ts: e.ts.clone(),
                            data: e.data.clone(),
                            memory: e.memory.clone(),
                        })
                        .collect())
                }
                _ => Ok(vec![]),
            },
            ValueType::List => match self.data.entries.get(key) {
                Some(DataValue::List { items, .. }) => {
                    let filtered: Vec<_> = items
                        .iter()
                        .filter(|e| {
                            range.is_none_or(|r| ts_in_range(&e.ts, r))
                                && where_matches(&e.data, where_clauses)
                        })
                        .collect();
                    let start = filtered.len().saturating_sub(count);
                    Ok(filtered[start..]
                        .iter()
                        .map(|e| SearchHit {
                            index: e.index,
                            id: e.id.clone(),
                            value: e.value.clone(),
                            ts: e.ts.clone(),
                            data: e.data.clone(),
                            memory: e.memory.clone(),
                        })
                        .collect())
                }
                _ => Ok(vec![]),
            },
            _ => Err(KvError::TypeMismatch {
                key: key.to_string(),
                expected: "history or list".to_string(),
                got: def.value_type.to_string(),
            }),
        }
    }

    /// Get random entries from a history or list as `SearchHit`s.
    ///
    /// When `range` is provided, entries are filtered by timestamp first, then
    /// `count` random items are sampled from the filtered set. If fewer entries
    /// are available than requested, all matching entries are returned and a note
    /// is printed to stderr.
    /// When `where_clauses` is non-empty, entries must match all clauses.
    pub fn random(
        &self,
        key: &str,
        count: usize,
        range: Option<&TimeRange>,
        where_clauses: &[(String, String)],
    ) -> Result<Vec<SearchHit>, KvError> {
        use rand::seq::IndexedRandom;

        let def = self.key_def(key)?;

        /// Shared sampling helper. Takes a filtered vec of `SearchHit` references
        /// and returns `count` randomly chosen hits (cloned).
        fn sample_hits(filtered: &[SearchHit], n: usize) -> Vec<SearchHit> {
            if filtered.is_empty() {
                return vec![];
            }
            let take = n.min(filtered.len());
            let mut rng = rand::rng();
            filtered.choose_multiple(&mut rng, take).cloned().collect()
        }

        match def.value_type {
            ValueType::History => match self.data.entries.get(key) {
                Some(DataValue::History { entries, .. }) => {
                    let filtered: Vec<SearchHit> = entries
                        .iter()
                        .filter(|e| {
                            range.is_none_or(|r| ts_in_range(&e.ts, r))
                                && where_matches(&e.data, where_clauses)
                        })
                        .map(|e| SearchHit {
                            index: e.index,
                            id: e.id.clone(),
                            value: e.value.clone(),
                            ts: e.ts.clone(),
                            data: e.data.clone(),
                            memory: e.memory.clone(),
                        })
                        .collect();
                    let available = filtered.len();
                    if available == 0 && !entries.is_empty() {
                        eprintln!("note: no entries match the time range");
                    } else if available > 0 && available < count {
                        eprintln!(
                            "note: only {} entries available (requested {})",
                            available, count
                        );
                    }
                    Ok(sample_hits(&filtered, count))
                }
                _ => Ok(vec![]),
            },
            ValueType::List => match self.data.entries.get(key) {
                Some(DataValue::List { items, .. }) => {
                    let filtered: Vec<SearchHit> = items
                        .iter()
                        .filter(|e| {
                            range.is_none_or(|r| ts_in_range(&e.ts, r))
                                && where_matches(&e.data, where_clauses)
                        })
                        .map(|e| SearchHit {
                            index: e.index,
                            id: e.id.clone(),
                            value: e.value.clone(),
                            ts: e.ts.clone(),
                            data: e.data.clone(),
                            memory: e.memory.clone(),
                        })
                        .collect();
                    let available = filtered.len();
                    if available == 0 && !items.is_empty() {
                        eprintln!("note: no entries match the time range");
                    } else if available > 0 && available < count {
                        eprintln!(
                            "note: only {} entries available (requested {})",
                            available, count
                        );
                    }
                    Ok(sample_hits(&filtered, count))
                }
                _ => Ok(vec![]),
            },
            _ => Err(KvError::TypeMismatch {
                key: key.to_string(),
                expected: "history or list".to_string(),
                got: def.value_type.to_string(),
            }),
        }
    }

    /// Get history entries since a given time reference.
    pub fn since(&self, key: &str, timeref: &str) -> Result<Vec<&HistoryEntry>, KvError> {
        self.assert_type(key, ValueType::History)?;

        let cutoff = parse_timeref(timeref).map_err(KvError::Other)?;

        match self.data.entries.get(key) {
            Some(DataValue::History { entries, .. }) => Ok(entries
                .iter()
                .filter(|e| {
                    DateTime::parse_from_rfc3339(&e.ts)
                        .map(|t| t >= cutoff)
                        .unwrap_or(false)
                })
                .collect()),
            _ => Ok(vec![]),
        }
    }

    /// Reset a key to its schema default.
    pub fn reset(&mut self, key: &str) -> Result<(), KvError> {
        let def = self.key_def(key)?.clone();
        self.data
            .entries
            .insert(key.to_string(), Self::default_value(&def));
        Ok(())
    }

    /// Remove entries from a list or history by value substring or by ID ref.
    ///
    /// - `by_id`: if Some, remove the entry matching that `IdRef` (ignores `value` and `all`).
    /// - `value`: substring match (case-insensitive).
    /// - `all`: if true, remove all matches; otherwise remove only the first match.
    pub fn remove(
        &mut self,
        key: &str,
        value: Option<&str>,
        by_id: Option<&IdRef>,
        all: bool,
    ) -> Result<RemoveResult, KvError> {
        let def = self.key_def(key)?;
        match def.value_type {
            ValueType::History | ValueType::List => {}
            _ => {
                return Err(KvError::TypeMismatch {
                    key: key.to_string(),
                    expected: "history or list".to_string(),
                    got: def.value_type.to_string(),
                });
            }
        }

        let mut removed = Vec::new();

        match self.data.entries.get_mut(key) {
            Some(DataValue::History { entries, .. }) => {
                if let Some(id_ref) = by_id {
                    let pos = match id_ref {
                        IdRef::Index(idx) => entries.iter().position(|e| e.index == *idx),
                        IdRef::Id(h) => {
                            let matches: Vec<usize> = entries
                                .iter()
                                .enumerate()
                                .filter(|(_, e)| e.id.starts_with(h.as_str()))
                                .map(|(i, _)| i)
                                .collect();
                            match matches.len() {
                                0 => None,
                                1 => Some(matches[0]),
                                n => {
                                    return Err(KvError::AmbiguousId {
                                        prefix: h.clone(),
                                        count: n,
                                    });
                                }
                            }
                        }
                    };
                    if let Some(pos) = pos {
                        removed.push(entries.remove(pos).value);
                    }
                } else if let Some(query) = value {
                    let query_lower = query.to_lowercase();
                    let mut found_first = false;
                    entries.retain(|e| {
                        if e.value.to_lowercase().contains(&query_lower) && (all || !found_first) {
                            found_first = true;
                            removed.push(e.value.clone());
                            return false;
                        }
                        true
                    });
                }
            }
            Some(DataValue::List { items, .. }) => {
                if let Some(id_ref) = by_id {
                    let pos = match id_ref {
                        IdRef::Index(idx) => items.iter().position(|e| e.index == *idx),
                        IdRef::Id(h) => {
                            let matches: Vec<usize> = items
                                .iter()
                                .enumerate()
                                .filter(|(_, e)| e.id.starts_with(h.as_str()))
                                .map(|(i, _)| i)
                                .collect();
                            match matches.len() {
                                0 => None,
                                1 => Some(matches[0]),
                                n => {
                                    return Err(KvError::AmbiguousId {
                                        prefix: h.clone(),
                                        count: n,
                                    });
                                }
                            }
                        }
                    };
                    if let Some(pos) = pos {
                        removed.push(items.remove(pos).value);
                    }
                } else if let Some(query) = value {
                    let query_lower = query.to_lowercase();
                    let mut found_first = false;
                    items.retain(|e| {
                        if e.value.to_lowercase().contains(&query_lower) && (all || !found_first) {
                            found_first = true;
                            removed.push(e.value.clone());
                            return false;
                        }
                        true
                    });
                }
            }
            _ => {} // no data yet, nothing to remove
        }

        Ok(RemoveResult { removed })
    }

    /// Search entries in a list or history by case-insensitive substring and/or
    /// structured data filters.
    ///
    /// When `query` is `Some`, entries must contain the substring.
    /// When `where_clauses` is non-empty, entries must match all clauses.
    /// When `range` is provided, only entries within the time range are searched.
    pub fn search(
        &self,
        key: &str,
        query: Option<&str>,
        range: Option<&TimeRange>,
        where_clauses: &[(String, String)],
    ) -> Result<Vec<SearchHit>, KvError> {
        let def = self.key_def(key)?;
        match def.value_type {
            ValueType::History | ValueType::List => {}
            _ => {
                return Err(KvError::TypeMismatch {
                    key: key.to_string(),
                    expected: "history or list".to_string(),
                    got: def.value_type.to_string(),
                });
            }
        }

        let query_lower = query.map(|q| q.to_lowercase());
        let mut hits = Vec::new();

        match self.data.entries.get(key) {
            Some(DataValue::History { entries, .. }) => {
                for e in entries {
                    if !range.is_none_or(|r| ts_in_range(&e.ts, r)) {
                        continue;
                    }
                    if let Some(ref q) = query_lower
                        && !e.value.to_lowercase().contains(q)
                    {
                        continue;
                    }
                    if !where_matches(&e.data, where_clauses) {
                        continue;
                    }
                    hits.push(SearchHit {
                        index: e.index,
                        id: e.id.clone(),
                        value: e.value.clone(),
                        ts: e.ts.clone(),
                        data: e.data.clone(),
                        memory: e.memory.clone(),
                    });
                }
            }
            Some(DataValue::List { items, .. }) => {
                for e in items {
                    if !range.is_none_or(|r| ts_in_range(&e.ts, r)) {
                        continue;
                    }
                    if let Some(ref q) = query_lower
                        && !e.value.to_lowercase().contains(q)
                    {
                        continue;
                    }
                    if !where_matches(&e.data, where_clauses) {
                        continue;
                    }
                    hits.push(SearchHit {
                        index: e.index,
                        id: e.id.clone(),
                        value: e.value.clone(),
                        ts: e.ts.clone(),
                        data: e.data.clone(),
                        memory: e.memory.clone(),
                    });
                }
            }
            _ => {}
        }

        Ok(hits)
    }

    /// Look up entries by index (numeric) or ID (stable ID) in a history or list.
    ///
    /// Returns matching entries as `SearchHit`s (same struct used by `search`).
    /// ID matching is prefix-based: `"A3f"` matches an entry with ID `"A3fBx2"`.
    /// Only works on History and List types; returns TypeMismatch for others.
    pub fn get_entries_by_id(&self, key: &str, ids: &[IdRef]) -> Result<Vec<SearchHit>, KvError> {
        let def = self.key_def(key)?;
        match def.value_type {
            ValueType::History | ValueType::List => {}
            _ => {
                return Err(KvError::TypeMismatch {
                    key: key.to_string(),
                    expected: "history or list".to_string(),
                    got: def.value_type.to_string(),
                });
            }
        }

        let numeric_indexes: HashSet<u64> = ids
            .iter()
            .filter_map(|r| match r {
                IdRef::Index(n) => Some(*n),
                _ => None,
            })
            .collect();
        let id_prefixes: Vec<&str> = ids
            .iter()
            .filter_map(|r| match r {
                IdRef::Id(h) => Some(h.as_str()),
                _ => None,
            })
            .collect();

        let matches_entry = |index: u64, id: &str| -> bool {
            if numeric_indexes.contains(&index) {
                return true;
            }
            id_prefixes.iter().any(|prefix| id.starts_with(prefix))
        };

        let mut hits = Vec::new();

        match self.data.entries.get(key) {
            Some(DataValue::History { entries, .. }) => {
                for e in entries {
                    if matches_entry(e.index, &e.id) {
                        hits.push(SearchHit {
                            index: e.index,
                            id: e.id.clone(),
                            value: e.value.clone(),
                            ts: e.ts.clone(),
                            data: e.data.clone(),
                            memory: e.memory.clone(),
                        });
                    }
                }
            }
            Some(DataValue::List { items, .. }) => {
                for e in items {
                    if matches_entry(e.index, &e.id) {
                        hits.push(SearchHit {
                            index: e.index,
                            id: e.id.clone(),
                            value: e.value.clone(),
                            ts: e.ts.clone(),
                            data: e.data.clone(),
                            memory: e.memory.clone(),
                        });
                    }
                }
            }
            _ => {} // Key defined in schema but no entries pushed yet
        }

        Ok(hits)
    }

    /// Count entries in a list or history, optionally filtered by substring,
    /// time range, and/or structured data filters.
    ///
    /// When `range` is provided, only entries within the time range are counted.
    /// When `where_clauses` is non-empty, entries must match all clauses.
    /// The `total` field in the result reflects entries that passed the time filter
    /// (when a value filter or where filter is active).
    pub fn count(
        &self,
        key: &str,
        value: Option<&str>,
        range: Option<&TimeRange>,
        where_clauses: &[(String, String)],
    ) -> Result<CountResult, KvError> {
        let def = self.key_def(key)?;
        match def.value_type {
            ValueType::History | ValueType::List => {}
            _ => {
                return Err(KvError::TypeMismatch {
                    key: key.to_string(),
                    expected: "history or list".to_string(),
                    got: def.value_type.to_string(),
                });
            }
        }

        let query_lower = value.map(|v| v.to_lowercase());
        let filtering = query_lower.is_some() || !where_clauses.is_empty();
        let mut matched = 0usize;
        let mut entry_total = 0usize;
        let mut latest_ts: Option<String> = None;

        match self.data.entries.get(key) {
            Some(DataValue::History { entries, .. }) => {
                for e in entries {
                    if !range.is_none_or(|r| ts_in_range(&e.ts, r)) {
                        continue;
                    }
                    entry_total += 1;
                    let text_match = match &query_lower {
                        Some(q) => e.value.to_lowercase().contains(q),
                        None => true,
                    };
                    let is_match = text_match && where_matches(&e.data, where_clauses);
                    if is_match {
                        matched += 1;
                        if latest_ts.is_none() || e.ts > *latest_ts.as_ref().unwrap() {
                            latest_ts = Some(e.ts.clone());
                        }
                    }
                }
            }
            Some(DataValue::List { items, .. }) => {
                for e in items {
                    if !range.is_none_or(|r| ts_in_range(&e.ts, r)) {
                        continue;
                    }
                    entry_total += 1;
                    let text_match = match &query_lower {
                        Some(q) => e.value.to_lowercase().contains(q),
                        None => true,
                    };
                    let is_match = text_match && where_matches(&e.data, where_clauses);
                    if is_match {
                        matched += 1;
                        if !e.ts.is_empty()
                            && (latest_ts.is_none() || e.ts > *latest_ts.as_ref().unwrap())
                        {
                            latest_ts = Some(e.ts.clone());
                        }
                    }
                }
            }
            _ => {}
        }

        Ok(CountResult {
            matched,
            total: if filtering { Some(entry_total) } else { None },
            latest_ts,
        })
    }

    /// List all keys with their types and optional descriptions.
    pub fn keys(&self) -> Vec<(&str, ValueType, Option<&str>)> {
        self.schema
            .keys
            .iter()
            .map(|(k, v)| (k.as_str(), v.value_type, v.description.as_deref()))
            .collect()
    }

    /// Dump all state as JSON.
    pub fn dump_json(&self) -> Result<String> {
        serde_json::to_string_pretty(&self.data).context("Failed to serialize data")
    }

    /// Dump all state in compact format for wake integration.
    pub fn dump_compact(&self) -> String {
        let mut parts = Vec::new();

        for (key, def) in &self.schema.keys {
            let part = match self.data.entries.get(key) {
                Some(val) => format_compact(key, val, def),
                None => format_compact(key, &Self::default_value(def), def),
            };
            parts.push(part);
        }

        parts.join(" ")
    }

    // -----------------------------------------------------------------------
    // Memory pointer operations
    // -----------------------------------------------------------------------

    /// Set the memory pointer (kn- reference) on a history, list, or state key.
    /// Pass `None` to clear the pointer.
    pub fn set_memory(&mut self, key: &str, memory: Option<String>) -> Result<(), KvError> {
        let def = self.key_def(key)?;
        match def.value_type {
            ValueType::History | ValueType::List | ValueType::State => {}
            _ => {
                return Err(KvError::TypeMismatch {
                    key: key.to_string(),
                    expected: "history, list, or state".to_string(),
                    got: def.value_type.to_string(),
                });
            }
        }

        let def = def.clone();
        let entry = self
            .data
            .entries
            .entry(key.to_string())
            .or_insert_with(|| Self::default_value(&def));

        // Normalize empty string to None (clearing the link)
        let memory = memory.filter(|s| !s.is_empty());

        match entry {
            DataValue::History { memory: mem, .. } => *mem = memory,
            DataValue::List { memory: mem, .. } => *mem = memory,
            DataValue::State { memory: mem, .. } => *mem = memory,
            _ => unreachable!(),
        }

        Ok(())
    }

    /// Get the memory pointer for a key, if set.
    pub fn get_memory(&self, key: &str) -> Result<Option<&str>, KvError> {
        self.key_def(key)?; // validate key exists in schema

        match self.data.entries.get(key) {
            Some(DataValue::History { memory, .. }) => Ok(memory.as_deref()),
            Some(DataValue::List { memory, .. }) => Ok(memory.as_deref()),
            Some(DataValue::State { memory, .. }) => Ok(memory.as_deref()),
            Some(DataValue::Counter { .. } | DataValue::String { .. }) => {
                Err(KvError::TypeMismatch {
                    key: key.to_string(),
                    expected: "history, list, or state".to_string(),
                    got: "counter or string".to_string(),
                })
            }
            None => Ok(None),
        }
    }

    /// Set or clear the memory pointer on a specific entry within a history or list.
    ///
    /// Pass `None` (or `Some("")`) to clear the pointer.
    /// Returns `KvError::EntryNotFound` if the entry is not found.
    pub fn set_entry_memory(
        &mut self,
        key: &str,
        id: &IdRef,
        memory: Option<String>,
    ) -> Result<(), KvError> {
        let def = self.key_def(key)?;
        match def.value_type {
            ValueType::History | ValueType::List => {}
            _ => {
                return Err(KvError::TypeMismatch {
                    key: key.to_string(),
                    expected: "history or list".to_string(),
                    got: def.value_type.to_string(),
                });
            }
        }

        // Normalize empty string to None (clearing the link)
        let memory = memory.filter(|s| !s.is_empty());

        match self.data.entries.get_mut(key) {
            Some(DataValue::History { entries, .. }) => {
                let entry = match id {
                    IdRef::Index(n) => entries.iter_mut().find(|e| e.index == *n),
                    IdRef::Id(h) => {
                        let matches: Vec<_> = entries
                            .iter_mut()
                            .filter(|e| e.id.starts_with(h.as_str()))
                            .collect();
                        match matches.len() {
                            0 => None,
                            1 => {
                                // Re-find to satisfy borrow checker (collected vec consumes &mut)
                                entries.iter_mut().find(|e| e.id.starts_with(h.as_str()))
                            }
                            n => {
                                return Err(KvError::AmbiguousId {
                                    prefix: h.clone(),
                                    count: n,
                                });
                            }
                        }
                    }
                };
                match entry {
                    Some(e) => {
                        e.memory = memory;
                        Ok(())
                    }
                    None => Err(KvError::EntryNotFound {
                        key: key.to_string(),
                        id: format!("{:?}", id),
                    }),
                }
            }
            Some(DataValue::List { items, .. }) => {
                let entry = match id {
                    IdRef::Index(n) => items.iter_mut().find(|e| e.index == *n),
                    IdRef::Id(h) => {
                        let matches: Vec<_> = items
                            .iter_mut()
                            .filter(|e| e.id.starts_with(h.as_str()))
                            .collect();
                        match matches.len() {
                            0 => None,
                            1 => items.iter_mut().find(|e| e.id.starts_with(h.as_str())),
                            n => {
                                return Err(KvError::AmbiguousId {
                                    prefix: h.clone(),
                                    count: n,
                                });
                            }
                        }
                    }
                };
                match entry {
                    Some(e) => {
                        e.memory = memory;
                        Ok(())
                    }
                    None => Err(KvError::EntryNotFound {
                        key: key.to_string(),
                        id: format!("{:?}", id),
                    }),
                }
            }
            _ => Err(KvError::EntryNotFound {
                key: key.to_string(),
                id: format!("{:?}", id),
            }),
        }
    }

    /// Update an existing entry in a history or list in-place.
    ///
    /// Preserves the entry's `index`, `id`, `ts`, and position in the list.
    /// When `new_value` is `Some`, replaces the entry's `value`.
    /// When `new_data` is `Some`, performs a shallow object merge into the existing
    /// `data` field. Null values in the patch *delete* the corresponding field from
    /// the merged object (not "set to null"). Validation runs on the merged result
    /// before the write commits.
    ///
    /// At least one of `new_value` or `new_data` must carry a meaningful update:
    /// passing `new_value = None` with `new_data = Some({})` (an empty object)
    /// is rejected here in the engine so library callers get the same guard as
    /// the CLI handler.
    pub fn update_entry(
        &mut self,
        key: &str,
        id: &IdRef,
        new_value: Option<&str>,
        new_data: Option<serde_json::Value>,
    ) -> Result<UpdateResult, KvError> {
        // Empty-object patch with no value change — nothing would be written.
        if new_value.is_none()
            && new_data
                .as_ref()
                .and_then(|d| d.as_object())
                .is_some_and(|o| o.is_empty())
        {
            return Err(KvError::DataValidation {
                message: format!(
                    "key '{}': --data is an empty object and no value was given — nothing to update",
                    key
                ),
            });
        }

        let def = self.key_def(key)?.clone();
        match def.value_type {
            ValueType::History | ValueType::List => {}
            _ => {
                return Err(KvError::TypeMismatch {
                    key: key.to_string(),
                    expected: "history or list".to_string(),
                    got: def.value_type.to_string(),
                });
            }
        }

        match self.data.entries.get_mut(key) {
            Some(DataValue::History { entries, .. }) => {
                apply_entry_update(entries, id, key, new_value, new_data.as_ref(), &def)
            }
            Some(DataValue::List { items, .. }) => {
                apply_entry_update(items, id, key, new_value, new_data.as_ref(), &def)
            }
            _ => Err(KvError::EntryNotFound {
                key: key.to_string(),
                id: format!("{:?}", id),
            }),
        }
    }

    // -----------------------------------------------------------------------
    // Helpers
    // -----------------------------------------------------------------------

    fn clamp(value: i64, min: Option<i64>, max: Option<i64>) -> i64 {
        let mut v = value;
        if let Some(lo) = min {
            v = v.max(lo);
        }
        if let Some(hi) = max {
            v = v.min(hi);
        }
        v
    }
}

// ---------------------------------------------------------------------------
// Structured data filter
// ---------------------------------------------------------------------------

/// Check whether an entry's structured data matches all `(key, value)` clauses.
///
/// Semantics:
/// - If `data` is `None` and clauses is non-empty, return `false`.
/// - For each `(key, value)` clause, `data[key]` must equal the string value
///   OR `data[key]` must be an array that contains the string value.
/// - ALL clauses must match (AND logic).
pub fn where_matches(data: &Option<serde_json::Value>, clauses: &[(String, String)]) -> bool {
    if clauses.is_empty() {
        return true;
    }

    let obj = match data {
        Some(serde_json::Value::Object(map)) => map,
        _ => return false,
    };

    clauses.iter().all(|(key, value)| match obj.get(key) {
        Some(serde_json::Value::String(s)) => s == value,
        Some(serde_json::Value::Array(arr)) => arr
            .iter()
            .any(|v| matches!(v, serde_json::Value::String(s) if s == value)),
        Some(serde_json::Value::Number(n)) => n.to_string() == *value,
        Some(serde_json::Value::Bool(b)) => b.to_string() == *value,
        _ => false,
    })
}

/// Format the data suffix for display output.
///
/// When data is `Some`, returns ` {compact_json}`. When `None`, returns empty string.
pub fn format_data_suffix(data: &Option<serde_json::Value>) -> String {
    match data {
        Some(v) => format!(" {}", serde_json::to_string(v).unwrap_or_default()),
        None => String::new(),
    }
}

// ---------------------------------------------------------------------------
// Compact format
// ---------------------------------------------------------------------------

fn format_compact(key: &str, value: &DataValue, def: &KeyDef) -> String {
    match value {
        DataValue::Counter { value } => format!("{}={}", key, value),
        DataValue::String { value } => format!("{}={}", key, value),
        DataValue::History {
            entries, memory, ..
        } => {
            let items: Vec<String> = entries
                .iter()
                .map(|e| {
                    let time_part = DateTime::parse_from_rfc3339(&e.ts)
                        .map(|t| t.format("%H:%M").to_string())
                        .unwrap_or_else(|_| "??:??".to_string());
                    format!("{}@{}", e.value, time_part)
                })
                .collect();
            let base = format!("{}=[{}]", key, items.join(","));
            match memory {
                Some(m) if !m.is_empty() => format!("{}({})", base, m),
                _ => base,
            }
        }
        DataValue::State { fields, memory, .. } => {
            // Values only, ordered by schema field order
            let values: Vec<String> = def
                .fields
                .as_ref()
                .map(|schema_fields| {
                    schema_fields
                        .iter()
                        .map(|f| fields.get(f).cloned().unwrap_or_default())
                        .collect()
                })
                .unwrap_or_else(|| fields.values().cloned().collect());
            let base = format!("{}={{{}}}", key, values.join(","));
            match memory {
                Some(m) if !m.is_empty() => format!("{}({})", base, m),
                _ => base,
            }
        }
        DataValue::List { items, memory, .. } => {
            let formatted: Vec<String> = items
                .iter()
                .map(|e| {
                    if e.ts.is_empty() {
                        e.value.clone()
                    } else {
                        let time_part = DateTime::parse_from_rfc3339(&e.ts)
                            .map(|t| t.format("%H:%M").to_string())
                            .unwrap_or_else(|_| "??:??".to_string());
                        format!("{}@{}", e.value, time_part)
                    }
                })
                .collect();
            let base = format!("{}=[{}]", key, formatted.join(","));
            match memory {
                Some(m) if !m.is_empty() => format!("{}({})", base, m),
                _ => base,
            }
        }
    }
}

// ---------------------------------------------------------------------------
// Relative time parser
// ---------------------------------------------------------------------------

/// Parse a time reference: either ISO-8601 or relative (1h, 7d, 2w, 30m).
pub fn parse_timeref(timeref: &str) -> Result<DateTime<Utc>> {
    // Try ISO-8601 first
    if let Ok(dt) = DateTime::parse_from_rfc3339(timeref) {
        return Ok(dt.with_timezone(&Utc));
    }

    // Try relative time
    parse_relative_time(timeref)
}

/// Parse a relative time string like "1h", "7d", "2w", "30m".
pub fn parse_relative_time(s: &str) -> Result<DateTime<Utc>> {
    let s = s.trim();
    if s.is_empty() {
        bail!("Empty time reference");
    }

    let (num_str, unit) = s.split_at(s.len() - 1);
    let num: i64 = num_str
        .parse()
        .with_context(|| format!("Invalid number in time reference: '{}'", s))?;

    let duration = match unit {
        "m" => chrono::Duration::minutes(num),
        "h" => chrono::Duration::hours(num),
        "d" => chrono::Duration::days(num),
        "w" => chrono::Duration::weeks(num),
        _ => bail!(
            "Unknown time unit '{}' in '{}'. Use m (minutes), h (hours), d (days), w (weeks)",
            unit,
            s
        ),
    };

    Ok(Utc::now() - duration)
}

// ---------------------------------------------------------------------------
// Time-range queries
// ---------------------------------------------------------------------------

/// Half-open time range `[from, to)` in UTC.
#[derive(Debug, Clone)]
pub struct TimeRange {
    /// Inclusive lower bound.
    pub from: DateTime<Utc>,
    /// Exclusive upper bound.
    pub to: DateTime<Utc>,
}

/// Parse `YYYY-MM-DD` into a day range `[start_of_day, start_of_next_day)`.
pub fn parse_day(s: &str) -> Result<TimeRange> {
    let date = NaiveDate::parse_from_str(s, "%Y-%m-%d")
        .with_context(|| format!("Invalid day format '{}', expected YYYY-MM-DD", s))?;
    let from = date.and_hms_opt(0, 0, 0).unwrap().and_utc();
    let to = date
        .succ_opt()
        .with_context(|| format!("Day overflow for '{}'", s))?
        .and_hms_opt(0, 0, 0)
        .unwrap()
        .and_utc();
    Ok(TimeRange { from, to })
}

/// Parse `YYYY-MM` into a month range `[first_of_month, first_of_next_month)`.
pub fn parse_month(s: &str) -> Result<TimeRange> {
    let parts: Vec<&str> = s.split('-').collect();
    if parts.len() != 2 {
        bail!("Invalid month format '{}', expected YYYY-MM", s);
    }
    let year: i32 = parts[0]
        .parse()
        .with_context(|| format!("Invalid year in month '{}'", s))?;
    let month: u32 = parts[1]
        .parse()
        .with_context(|| format!("Invalid month number in '{}'", s))?;
    if !(1..=12).contains(&month) {
        bail!("Month out of range in '{}'", s);
    }

    let from_date = NaiveDate::from_ymd_opt(year, month, 1)
        .with_context(|| format!("Invalid month '{}'", s))?;
    let from = from_date.and_hms_opt(0, 0, 0).unwrap().and_utc();

    // First of next month (handle December -> January rollover)
    let (next_year, next_month) = if month == 12 {
        (year + 1, 1)
    } else {
        (year, month + 1)
    };
    let to_date = NaiveDate::from_ymd_opt(next_year, next_month, 1)
        .with_context(|| format!("Month overflow for '{}'", s))?;
    let to = to_date.and_hms_opt(0, 0, 0).unwrap().and_utc();

    Ok(TimeRange { from, to })
}

/// Parse `YYYY-Www` (ISO week) into a range `[Monday, next_Monday)`.
pub fn parse_week(s: &str) -> Result<TimeRange> {
    // Expect format like "2026-W17"
    let parts: Vec<&str> = s.split("-W").collect();
    if parts.len() != 2 {
        bail!(
            "Invalid week format '{}', expected YYYY-Www (e.g. 2026-W17)",
            s
        );
    }
    let year: i32 = parts[0]
        .parse()
        .with_context(|| format!("Invalid year in week '{}'", s))?;
    let week: u32 = parts[1]
        .parse()
        .with_context(|| format!("Invalid week number in '{}'", s))?;
    if week == 0 || week > 53 {
        bail!("Week number out of range in '{}' (must be 1-53)", s);
    }

    let from_date = NaiveDate::from_isoywd_opt(year, week, chrono::Weekday::Mon)
        .with_context(|| format!("Invalid ISO week '{}'", s))?;
    let from = from_date.and_hms_opt(0, 0, 0).unwrap().and_utc();

    let to_date = from_date + chrono::Duration::days(7);
    let to = to_date.and_hms_opt(0, 0, 0).unwrap().and_utc();

    Ok(TimeRange { from, to })
}

/// Parse an explicit `--from`/`--to` date range.
///
/// - Both provided: `[from, to+1day)` (inclusive of the to-date).
/// - `from` only: `[from, now)`.
/// - `to` only: `[epoch, to+1day)`.
pub fn parse_date_range(from: Option<&str>, to: Option<&str>) -> Result<TimeRange> {
    let range_from = match from {
        Some(s) => {
            let date = NaiveDate::parse_from_str(s, "%Y-%m-%d")
                .with_context(|| format!("Invalid --from date '{}', expected YYYY-MM-DD", s))?;
            date.and_hms_opt(0, 0, 0).unwrap().and_utc()
        }
        None => {
            // Beginning of time (Unix epoch)
            DateTime::UNIX_EPOCH
        }
    };

    let range_to = match to {
        Some(s) => {
            let date = NaiveDate::parse_from_str(s, "%Y-%m-%d")
                .with_context(|| format!("Invalid --to date '{}', expected YYYY-MM-DD", s))?;
            // Inclusive: end of that day = start of next day
            let next = date
                .succ_opt()
                .with_context(|| format!("Date overflow for --to '{}'", s))?;
            next.and_hms_opt(0, 0, 0).unwrap().and_utc()
        }
        None => Utc::now(),
    };

    if range_from >= range_to {
        bail!(
            "--from ({}) must be before --to ({})",
            range_from.format("%Y-%m-%d"),
            range_to.format("%Y-%m-%d")
        );
    }

    Ok(TimeRange {
        from: range_from,
        to: range_to,
    })
}

/// Top-level resolver: convert CLI `TimeRangeArgs` into an optional `TimeRange`.
///
/// Returns `Ok(None)` when no time-range flags were provided.
pub fn resolve_time_range(args: &TimeRangeArgs) -> Result<Option<TimeRange>> {
    if let Some(ref day) = args.day {
        return parse_day(day).map(Some);
    }
    if let Some(ref month) = args.month {
        return parse_month(month).map(Some);
    }
    if let Some(ref week) = args.week {
        return parse_week(week).map(Some);
    }
    if let Some(ref since) = args.since {
        let from = parse_timeref(since)?;
        let to = Utc::now();
        return Ok(Some(TimeRange { from, to }));
    }
    if args.range_from.is_some() || args.range_to.is_some() {
        return parse_date_range(args.range_from.as_deref(), args.range_to.as_deref()).map(Some);
    }
    Ok(None)
}

/// Check whether a timestamp string falls within a `TimeRange`.
///
/// Unparseable or empty timestamps never match.
pub fn ts_in_range(ts: &str, range: &TimeRange) -> bool {
    DateTime::parse_from_rfc3339(ts)
        .map(|t| {
            let t = t.with_timezone(&Utc);
            t >= range.from && t < range.to
        })
        .unwrap_or(false)
}

// ---------------------------------------------------------------------------
// Display helpers (for CLI output)
// ---------------------------------------------------------------------------

/// Format a single entry line with numeric index, ID, value, optional timestamp, and data suffix.
pub fn format_entry_line(
    index: u64,
    id: &str,
    value: &str,
    ts: &str,
    data: &Option<serde_json::Value>,
) -> String {
    let data_suffix = format_data_suffix(data);
    if ts.is_empty() {
        format!("{} [kv-{}]: {}{}", index, id, value, data_suffix)
    } else {
        format!("{} [kv-{}]: {} ({}){}", index, id, value, ts, data_suffix)
    }
}

/// Format a DataValue for human-readable CLI output.
pub fn format_value(value: &DataValue) -> String {
    match value {
        DataValue::Counter { value } => value.to_string(),
        DataValue::String { value } => value.clone(),
        DataValue::History { entries, .. } => entries
            .iter()
            .map(|e| format_entry_line(e.index, &e.id, &e.value, &e.ts, &e.data))
            .collect::<Vec<_>>()
            .join("\n"),
        DataValue::State { fields, .. } => {
            serde_json::to_string_pretty(fields).unwrap_or_else(|_| "{}".to_string())
        }
        DataValue::List { items, .. } => items
            .iter()
            .map(|e| format_entry_line(e.index, &e.id, &e.value, &e.ts, &e.data))
            .collect::<Vec<_>>()
            .join("\n"),
    }
}

// ---------------------------------------------------------------------------
// Tests
// ---------------------------------------------------------------------------

#[cfg(test)]
mod tests {
    use super::*;
    use std::io::Write as IoWrite;
    use tempfile::TempDir;

    fn test_schema() -> &'static str {
        r#"
[keys.warmth]
type = "counter"
min = 0
default = "0"

[keys.capped]
type = "counter"
min = 0
max = 100
default = "50"

[keys.flavor_history]
type = "history"
max_entries = 3

[keys.tensor]
type = "state"
fields = ["temperature", "entropy", "agency"]

[keys.current_mood]
type = "string"
default = "neutral"

[keys.tags]
type = "list"
max_entries = 5
"#
    }

    fn setup_store(schema_toml: &str) -> (KvStore, TempDir) {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(schema_toml.as_bytes()).unwrap();

        let store = KvStore::load(&schema_path, &data_path).unwrap();
        (store, dir)
    }

    // -- Schema parsing --

    #[test]
    fn parse_schema_all_types() {
        let (store, _dir) = setup_store(test_schema());
        assert_eq!(store.schema.keys.len(), 6);
        assert_eq!(store.schema.keys["warmth"].value_type, ValueType::Counter);
        assert_eq!(
            store.schema.keys["flavor_history"].value_type,
            ValueType::History
        );
        assert_eq!(store.schema.keys["tensor"].value_type, ValueType::State);
        assert_eq!(
            store.schema.keys["current_mood"].value_type,
            ValueType::String
        );
        assert_eq!(store.schema.keys["tags"].value_type, ValueType::List);
    }

    #[test]
    fn parse_schema_counter_constraints() {
        let (store, _dir) = setup_store(test_schema());
        let warmth = &store.schema.keys["warmth"];
        assert_eq!(warmth.min, Some(0));
        assert_eq!(warmth.max, None);

        let capped = &store.schema.keys["capped"];
        assert_eq!(capped.min, Some(0));
        assert_eq!(capped.max, Some(100));
        assert_eq!(capped.default.as_deref(), Some("50"));
    }

    #[test]
    fn parse_schema_state_fields() {
        let (store, _dir) = setup_store(test_schema());
        let tensor = &store.schema.keys["tensor"];
        assert_eq!(
            tensor.fields.as_ref().unwrap(),
            &["temperature", "entropy", "agency"]
        );
    }

    // -- Counter operations --

    #[test]
    fn counter_inc_dec() {
        let (mut store, _dir) = setup_store(test_schema());
        assert_eq!(store.inc("warmth", 1).unwrap(), 1);
        assert_eq!(store.inc("warmth", 5).unwrap(), 6);
        assert_eq!(store.dec("warmth", 2).unwrap(), 4);
    }

    #[test]
    fn counter_clamp_min() {
        let (mut store, _dir) = setup_store(test_schema());
        // warmth has min=0, default=0
        assert_eq!(store.dec("warmth", 100).unwrap(), 0);
    }

    #[test]
    fn counter_clamp_max() {
        let (mut store, _dir) = setup_store(test_schema());
        // capped has min=0, max=100, default=50
        assert_eq!(store.inc("capped", 1).unwrap(), 51);
        assert_eq!(store.inc("capped", 100).unwrap(), 100);
    }

    #[test]
    fn counter_set_and_clamp() {
        let (mut store, _dir) = setup_store(test_schema());
        store.set("capped", "200", None).unwrap();
        match store.get("capped").unwrap() {
            DataValue::Counter { value } => assert_eq!(*value, 100),
            _ => panic!("Expected counter"),
        }
    }

    // -- String operations --

    #[test]
    fn string_set_get() {
        let (mut store, _dir) = setup_store(test_schema());
        store.set("current_mood", "elated", None).unwrap();
        match store.get("current_mood").unwrap() {
            DataValue::String { value } => assert_eq!(value, "elated"),
            _ => panic!("Expected string"),
        }
    }

    // -- State operations --

    #[test]
    fn state_set_field() {
        let (mut store, _dir) = setup_store(test_schema());
        store.set("tensor", "0.75", Some("temperature")).unwrap();
        store.set("tensor", "0.30", Some("entropy")).unwrap();
        match store.get("tensor").unwrap() {
            DataValue::State { fields, .. } => {
                assert_eq!(fields["temperature"], "0.75");
                assert_eq!(fields["entropy"], "0.30");
            }
            _ => panic!("Expected state"),
        }
    }

    #[test]
    fn state_rejects_unknown_field() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set("tensor", "0.5", Some("nonexistent"));
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Unknown field"));
    }

    #[test]
    fn state_requires_field_name() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set("tensor", "0.5", None);
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("field name"));
    }

    // -- Batch state operations --

    #[test]
    fn batch_set_multiple_fields() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .set_state_batch(
                "tensor",
                &[
                    ("temperature".to_string(), "0.8".to_string()),
                    ("entropy".to_string(), "0.3".to_string()),
                    ("agency".to_string(), "0.9".to_string()),
                ],
            )
            .unwrap();
        match store.get("tensor").unwrap() {
            DataValue::State { fields, .. } => {
                assert_eq!(fields["temperature"], "0.8");
                assert_eq!(fields["entropy"], "0.3");
                assert_eq!(fields["agency"], "0.9");
            }
            _ => panic!("Expected state"),
        }
    }

    #[test]
    fn batch_preserves_existing_fields() {
        let (mut store, _dir) = setup_store(test_schema());
        // Set one field first
        store.set("tensor", "0.5", Some("temperature")).unwrap();
        // Batch update only entropy
        store
            .set_state_batch("tensor", &[("entropy".to_string(), "0.7".to_string())])
            .unwrap();
        match store.get("tensor").unwrap() {
            DataValue::State { fields, .. } => {
                assert_eq!(fields["temperature"], "0.5"); // preserved
                assert_eq!(fields["entropy"], "0.7"); // updated
            }
            _ => panic!("Expected state"),
        }
    }

    #[test]
    fn batch_rejects_unknown_field() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set_state_batch(
            "tensor",
            &[
                ("temperature".to_string(), "0.5".to_string()),
                ("bogus".to_string(), "0.1".to_string()),
            ],
        );
        assert!(result.is_err());
        let msg = result.unwrap_err().to_string();
        assert!(msg.contains("unknown field 'bogus'"), "got: {}", msg);
        // Verify nothing was written (entire batch rejected)
        assert!(!store.data.entries.contains_key("tensor"));
    }

    #[test]
    fn batch_rejects_empty_fields() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set_state_batch("tensor", &[]);
        assert!(result.is_err());
        let msg = result.unwrap_err().to_string();
        assert!(msg.contains("at least one field"), "got: {}", msg);
    }

    #[test]
    fn batch_rejects_duplicate_field_names() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set_state_batch(
            "tensor",
            &[
                ("temperature".to_string(), "0.5".to_string()),
                ("temperature".to_string(), "0.9".to_string()),
            ],
        );
        assert!(result.is_err());
        let msg = result.unwrap_err().to_string();
        assert!(msg.contains("duplicate"), "got: {}", msg);
    }

    #[test]
    fn batch_on_wrong_type_returns_type_mismatch() {
        let (mut store, _dir) = setup_store(test_schema());
        let result =
            store.set_state_batch("warmth", &[("temperature".to_string(), "0.5".to_string())]);
        assert!(result.is_err());
        let msg = result.unwrap_err().to_string();
        assert!(msg.contains("Type mismatch"), "got: {}", msg);
    }

    #[test]
    fn batch_overwrites_existing_field_value() {
        let (mut store, _dir) = setup_store(test_schema());
        store.set("tensor", "0.5", Some("temperature")).unwrap();
        store
            .set_state_batch("tensor", &[("temperature".to_string(), "0.9".to_string())])
            .unwrap();
        match store.get("tensor").unwrap() {
            DataValue::State { fields, .. } => {
                assert_eq!(fields["temperature"], "0.9");
            }
            _ => panic!("Expected state"),
        }
    }

    #[test]
    fn batch_reports_all_unknown_fields() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set_state_batch(
            "tensor",
            &[
                ("gol".to_string(), "value".to_string()),
                ("statis".to_string(), "value".to_string()),
            ],
        );
        assert!(result.is_err());
        let msg = result.unwrap_err().to_string();
        assert!(msg.contains("gol"), "got: {}", msg);
        assert!(msg.contains("statis"), "got: {}", msg);
    }

    // -- Tensor positional set --

    #[test]
    fn tensor_positional_set() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .set_tensor_batch(
                "tensor",
                &["0.4".to_string(), "0.6".to_string(), "0.5".to_string()],
            )
            .unwrap();
        match store.get("tensor").unwrap() {
            DataValue::State { fields, .. } => {
                assert_eq!(fields["temperature"], "0.4");
                assert_eq!(fields["entropy"], "0.6");
                assert_eq!(fields["agency"], "0.5");
            }
            _ => panic!("Expected state"),
        }
    }

    #[test]
    fn tensor_wrong_count() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set_tensor_batch(
            "tensor",
            &[
                "0.1".to_string(),
                "0.2".to_string(),
                "0.3".to_string(),
                "0.4".to_string(),
                "0.5".to_string(),
            ],
        );
        assert!(result.is_err());
        let msg = result.unwrap_err().to_string();
        assert!(msg.contains("expected 3 values"), "got: {}", msg);
        assert!(msg.contains("got 5"), "got: {}", msg);
    }

    #[test]
    fn tensor_on_key_without_fields() {
        // Create a state key without fields defined
        let schema = r#"
[keys.bare_state]
type = "state"
"#;
        let (mut store, _dir) = setup_store(schema);
        let result = store.set_tensor_batch("bare_state", &["0.1".to_string()]);
        assert!(result.is_err());
        let msg = result.unwrap_err().to_string();
        assert!(msg.contains("no fields defined"), "got: {}", msg);
    }

    #[test]
    fn tensor_on_wrong_type() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set_tensor_batch("warmth", &["0.5".to_string()]);
        assert!(result.is_err());
        let msg = result.unwrap_err().to_string();
        // Counter type has no fields, so tensor set fails at the fields-exist check
        assert!(msg.contains("no fields defined"), "got: {}", msg);
    }

    // -- History operations --

    #[test]
    fn history_push_and_last() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();
        store.push("flavor_history", "lapsang", None, None).unwrap();

        let last = store.last("flavor_history", 1, None, &[]).unwrap();
        assert_eq!(last.len(), 1);
        assert_eq!(last[0].value, "lapsang");

        let last2 = store.last("flavor_history", 2, None, &[]).unwrap();
        assert_eq!(last2.len(), 2);
    }

    #[test]
    fn history_max_entries_overflow() {
        let (mut store, _dir) = setup_store(test_schema());
        // max_entries = 3
        store.push("flavor_history", "a", None, None).unwrap();
        store.push("flavor_history", "b", None, None).unwrap();
        store.push("flavor_history", "c", None, None).unwrap();
        store.push("flavor_history", "d", None, None).unwrap();

        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                assert_eq!(entries.len(), 3);
                assert_eq!(entries[0].value, "d"); // newest first
                assert_eq!(entries[2].value, "b"); // oldest kept
                // Indexes should be assigned
                assert!(entries[0].index > 0);
            }
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn history_since() {
        let (mut store, _dir) = setup_store(test_schema());

        let old_time = Utc::now() - chrono::Duration::hours(3);
        let new_time = Utc::now() - chrono::Duration::minutes(10);

        store
            .push_with_ts("flavor_history", "old_one", old_time, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "new_one", new_time, None, None)
            .unwrap();

        let results = store.since("flavor_history", "1h").unwrap();
        assert_eq!(results.len(), 1);
        assert_eq!(results[0].value, "new_one");
    }

    // -- List operations --

    #[test]
    fn list_push_pop_last() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();
        store.push("tags", "gamma", None, None).unwrap();

        let last = store.last("tags", 2, None, &[]).unwrap();
        assert_eq!(last.len(), 2);
        assert_eq!(last[0].value, "beta");
        assert_eq!(last[1].value, "gamma");

        let popped = store.pop("tags").unwrap();
        assert!(popped.is_some());
        assert_eq!(popped.unwrap().value, "gamma");
    }

    #[test]
    fn list_max_entries_overflow() {
        let (mut store, _dir) = setup_store(test_schema());
        // max_entries = 5
        for i in 0..8 {
            store
                .push("tags", &format!("item_{}", i), None, None)
                .unwrap();
        }
        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 5);
                assert_eq!(items[0].value, "item_3"); // oldest kept
                assert_eq!(items[4].value, "item_7"); // newest
            }
            _ => panic!("Expected list"),
        }
    }

    // -- Type mismatch --

    #[test]
    fn type_mismatch_inc_on_string() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.inc("current_mood", 1);
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn type_mismatch_push_on_counter() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("warmth", "value", None, None);
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn type_mismatch_pop_on_history() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.pop("flavor_history");
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn type_mismatch_since_on_list() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.since("tags", "1h");
        assert!(result.is_err());
    }

    // -- Unknown key --

    #[test]
    fn unknown_key_rejected() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.get("nonexistent");
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Unknown key"));
    }

    // -- Reset --

    #[test]
    fn reset_counter() {
        let (mut store, _dir) = setup_store(test_schema());
        store.inc("warmth", 42).unwrap();
        store.reset("warmth").unwrap();
        match store.get("warmth").unwrap() {
            DataValue::Counter { value } => assert_eq!(*value, 0),
            _ => panic!("Expected counter"),
        }
    }

    #[test]
    fn reset_history() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("flavor_history", "test", None, None).unwrap();
        store.reset("flavor_history").unwrap();
        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => assert!(entries.is_empty()),
            _ => panic!("Expected history"),
        }
    }

    // -- Keys listing --

    #[test]
    fn keys_lists_all() {
        let (store, _dir) = setup_store(test_schema());
        let keys = store.keys();
        assert_eq!(keys.len(), 6);
        // All test_schema keys lack descriptions
        for (_name, _vtype, desc) in &keys {
            assert!(desc.is_none());
        }
    }

    #[test]
    fn key_description_round_trips() {
        let schema = r#"
[keys.mood]
type = "string"
description = "Current emotional state"

[keys.score]
type = "counter"
"#;
        let (store, _dir) = setup_store(schema);
        let keys = store.keys();
        let mood = keys.iter().find(|(k, _, _)| *k == "mood").unwrap();
        assert_eq!(mood.2, Some("Current emotional state"));
        let score = keys.iter().find(|(k, _, _)| *k == "score").unwrap();
        assert_eq!(score.2, None);
    }

    // -- Atomic write --

    #[test]
    fn atomic_write_round_trip() {
        let (mut store, _dir) = setup_store(test_schema());
        store.data.schema_id = "test".to_string();
        store.inc("warmth", 7).unwrap();
        store.set("current_mood", "happy", None).unwrap();
        store
            .push("flavor_history", "earl grey", None, None)
            .unwrap();
        store.save().unwrap();

        // Reload and verify
        let data_str = fs::read_to_string(&store.data_path).unwrap();
        let reloaded: DataFile = serde_json::from_str(&data_str).unwrap();
        assert_eq!(reloaded.schema_id, "test");
        match &reloaded.entries["warmth"] {
            DataValue::Counter { value } => assert_eq!(*value, 7),
            _ => panic!("Expected counter"),
        }
    }

    // -- Compact dump --

    #[test]
    fn compact_dump_format() {
        let (mut store, _dir) = setup_store(test_schema());
        store.inc("warmth", 42).unwrap();
        store.set("current_mood", "calm", None).unwrap();
        store.set("tensor", "0.55", Some("temperature")).unwrap();
        store.set("tensor", "0.35", Some("entropy")).unwrap();
        store.set("tensor", "0.70", Some("agency")).unwrap();

        let ts = DateTime::parse_from_rfc3339("2026-04-22T10:30:00Z")
            .unwrap()
            .with_timezone(&Utc);
        store.push_with_ts("tags", "focus", ts, None, None).unwrap();
        store.push_with_ts("tags", "rust", ts, None, None).unwrap();

        let compact = store.dump_compact();

        assert!(compact.contains("warmth=42"));
        assert!(compact.contains("current_mood=calm"));
        assert!(compact.contains("tensor={0.55,0.35,0.70}"));
        assert!(compact.contains("tags=[focus@10:30,rust@10:30]"));
    }

    #[test]
    fn compact_dump_counter_default() {
        let (store, _dir) = setup_store(test_schema());
        let compact = store.dump_compact();
        // Defaults when no data set
        assert!(compact.contains("warmth=0"));
        assert!(compact.contains("capped=50"));
    }

    #[test]
    fn compact_dump_history_with_times() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts = DateTime::parse_from_rfc3339("2026-04-22T19:13:00Z")
            .unwrap()
            .with_timezone(&Utc);
        store
            .push_with_ts("flavor_history", "bergamot", ts, None, None)
            .unwrap();

        let compact = store.dump_compact();
        assert!(compact.contains("flavor_history=[bergamot@19:13]"));
    }

    // -- Relative time parser --

    #[test]
    fn parse_relative_minutes() {
        let result = parse_relative_time("30m").unwrap();
        let diff = Utc::now() - result;
        // Should be approximately 30 minutes
        assert!(diff.num_minutes() >= 29 && diff.num_minutes() <= 31);
    }

    #[test]
    fn parse_relative_hours() {
        let result = parse_relative_time("2h").unwrap();
        let diff = Utc::now() - result;
        assert!(diff.num_hours() >= 1 && diff.num_hours() <= 3);
    }

    #[test]
    fn parse_relative_days() {
        let result = parse_relative_time("7d").unwrap();
        let diff = Utc::now() - result;
        assert!(diff.num_days() >= 6 && diff.num_days() <= 8);
    }

    #[test]
    fn parse_relative_weeks() {
        let result = parse_relative_time("2w").unwrap();
        let diff = Utc::now() - result;
        assert!(diff.num_weeks() >= 1 && diff.num_weeks() <= 3);
    }

    #[test]
    fn parse_relative_invalid_unit() {
        assert!(parse_relative_time("5x").is_err());
    }

    #[test]
    fn parse_relative_invalid_number() {
        assert!(parse_relative_time("abch").is_err());
    }

    #[test]
    fn parse_relative_empty() {
        assert!(parse_relative_time("").is_err());
    }

    #[test]
    fn parse_timeref_iso8601() {
        let result = parse_timeref("2026-04-22T19:13:00Z").unwrap();
        assert_eq!(result.year(), 2026);
    }

    #[test]
    fn parse_timeref_relative_fallback() {
        let result = parse_timeref("1h").unwrap();
        let diff = Utc::now() - result;
        assert!(diff.num_hours() >= 0 && diff.num_hours() <= 2);
    }

    // -- Edge cases --

    #[test]
    fn inc_by_custom_amount() {
        let (mut store, _dir) = setup_store(test_schema());
        assert_eq!(store.inc("warmth", 10).unwrap(), 10);
        assert_eq!(store.dec("warmth", 3).unwrap(), 7);
    }

    #[test]
    fn last_on_empty_returns_empty() {
        let (store, _dir) = setup_store(test_schema());
        let last = store.last("flavor_history", 5, None, &[]).unwrap();
        assert!(last.is_empty());
    }

    #[test]
    fn pop_on_empty_returns_none() {
        let (store, _dir) = setup_store(test_schema());
        // Don't push anything — pop should be None on empty
        // Need mutable for pop
        let mut store = store;
        let popped = store.pop("tags").unwrap();
        assert!(popped.is_none());
    }

    // -- Remove operations --

    #[test]
    fn remove_list_by_value_first_match() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();
        store.push("tags", "alpha-2", None, None).unwrap();

        let result = store
            .remove("tags", Some("alpha"), None::<&IdRef>, false)
            .unwrap();
        assert_eq!(result.removed.len(), 1);
        assert_eq!(result.removed[0], "alpha");

        // alpha-2 should still be there
        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 2);
                assert_eq!(items[0].value, "beta");
                assert_eq!(items[1].value, "alpha-2");
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn remove_list_by_value_all_matches() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();
        store.push("tags", "alpha-2", None, None).unwrap();

        let result = store
            .remove("tags", Some("alpha"), None::<&IdRef>, true)
            .unwrap();
        assert_eq!(result.removed.len(), 2);

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 1);
                assert_eq!(items[0].value, "beta");
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn remove_list_by_id() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();
        store.push("tags", "gamma", None, None).unwrap();

        // Get the index of "beta"
        let beta_idx = match store.get("tags").unwrap() {
            DataValue::List { items, .. } => items[1].index,
            _ => panic!("Expected list"),
        };

        let result = store
            .remove("tags", None, Some(&IdRef::Index(beta_idx)), false)
            .unwrap();
        assert_eq!(result.removed.len(), 1);
        assert_eq!(result.removed[0], "beta");
    }

    #[test]
    fn remove_history_by_value() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();
        store.push("flavor_history", "lapsang", None, None).unwrap();
        store
            .push("flavor_history", "bergamot vanilla", None, None)
            .unwrap();

        let result = store
            .remove("flavor_history", Some("bergamot"), None::<&IdRef>, false)
            .unwrap();
        assert_eq!(result.removed.len(), 1);

        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                assert_eq!(entries.len(), 2);
            }
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn remove_case_insensitive() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "Alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();

        let result = store
            .remove("tags", Some("alpha"), None::<&IdRef>, false)
            .unwrap();
        assert_eq!(result.removed.len(), 1);
        assert_eq!(result.removed[0], "Alpha");
    }

    #[test]
    fn remove_type_mismatch() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.remove("warmth", Some("x"), None::<&IdRef>, false);
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    // -- Search operations --

    #[test]
    fn search_list() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "rust-lang", None, None).unwrap();
        store.push("tags", "focus", None, None).unwrap();
        store.push("tags", "rust-tools", None, None).unwrap();

        let hits = store.search("tags", Some("rust"), None, &[]).unwrap();
        assert_eq!(hits.len(), 2);
        assert_eq!(hits[0].value, "rust-lang");
        assert_eq!(hits[1].value, "rust-tools");
    }

    #[test]
    fn search_history() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();
        store.push("flavor_history", "lapsang", None, None).unwrap();
        store
            .push("flavor_history", "bergamot vanilla", None, None)
            .unwrap();

        let hits = store
            .search("flavor_history", Some("bergamot"), None, &[])
            .unwrap();
        assert_eq!(hits.len(), 2);
    }

    #[test]
    fn search_case_insensitive() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "Rust", None, None).unwrap();
        store.push("tags", "RUST-tools", None, None).unwrap();
        store.push("tags", "python", None, None).unwrap();

        let hits = store.search("tags", Some("rust"), None, &[]).unwrap();
        assert_eq!(hits.len(), 2);
    }

    #[test]
    fn search_no_matches() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();

        let hits = store.search("tags", Some("zzz"), None, &[]).unwrap();
        assert!(hits.is_empty());
    }

    #[test]
    fn search_type_mismatch() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.search("warmth", Some("x"), None, &[]);
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    // -- get_entries_by_id operations --

    #[test]
    fn get_by_id_single_history() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();
        store.push("flavor_history", "lapsang", None, None).unwrap();
        store
            .push("flavor_history", "earl grey", None, None)
            .unwrap();

        // Get the index of the second entry
        let target_idx = match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => entries[1].index,
            _ => panic!("Expected history"),
        };

        let hits = store
            .get_entries_by_id("flavor_history", &[IdRef::Index(target_idx)])
            .unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].index, target_idx);
        assert_eq!(hits[0].value, "lapsang");
    }

    #[test]
    fn get_by_id_single_list() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();
        store.push("tags", "gamma", None, None).unwrap();

        // Read the actual index of the second entry from the store
        let target_idx = match store.get("tags").unwrap() {
            DataValue::List { items, .. } => items[1].index,
            _ => panic!("Expected list"),
        };

        let hits = store
            .get_entries_by_id("tags", &[IdRef::Index(target_idx)])
            .unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].index, target_idx);
        assert_eq!(hits[0].value, "beta");
    }

    #[test]
    fn get_by_id_range_list() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();
        store.push("tags", "gamma", None, None).unwrap();

        let hits = store
            .get_entries_by_id("tags", &[IdRef::Index(1), IdRef::Index(2), IdRef::Index(3)])
            .unwrap();
        assert_eq!(hits.len(), 3);
        assert_eq!(hits[0].value, "alpha");
        assert_eq!(hits[1].value, "beta");
        assert_eq!(hits[2].value, "gamma");
    }

    #[test]
    fn get_by_id_multi_list() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();
        store.push("tags", "gamma", None, None).unwrap();

        let hits = store
            .get_entries_by_id("tags", &[IdRef::Index(1), IdRef::Index(3)])
            .unwrap();
        assert_eq!(hits.len(), 2);
        assert_eq!(hits[0].value, "alpha");
        assert_eq!(hits[1].value, "gamma");
    }

    #[test]
    fn get_by_id_partial_match() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();

        // Request IDs 1, 2, 99 — only 1 and 2 exist
        let hits = store
            .get_entries_by_id(
                "tags",
                &[IdRef::Index(1), IdRef::Index(2), IdRef::Index(99)],
            )
            .unwrap();
        assert_eq!(hits.len(), 2);
    }

    #[test]
    fn get_by_id_all_not_found() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();

        let hits = store
            .get_entries_by_id("tags", &[IdRef::Index(99), IdRef::Index(100)])
            .unwrap();
        assert!(hits.is_empty());
    }

    #[test]
    fn get_by_id_type_mismatch_counter() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.get_entries_by_id("warmth", &[IdRef::Index(1)]);
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn get_by_id_type_mismatch_string() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.get_entries_by_id("current_mood", &[IdRef::Index(1)]);
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn get_by_id_type_mismatch_state() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.get_entries_by_id("tensor", &[IdRef::Index(1)]);
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn get_by_id_empty_history() {
        let (store, _dir) = setup_store(test_schema());
        let hits = store
            .get_entries_by_id("flavor_history", &[IdRef::Index(1)])
            .unwrap();
        assert!(hits.is_empty());
    }

    #[test]
    fn get_by_id_empty_list() {
        let (store, _dir) = setup_store(test_schema());
        let hits = store.get_entries_by_id("tags", &[IdRef::Index(1)]).unwrap();
        assert!(hits.is_empty());
    }

    // -- Count operations --

    #[test]
    fn count_list_total() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "a", None, None).unwrap();
        store.push("tags", "b", None, None).unwrap();
        store.push("tags", "c", None, None).unwrap();

        let result = store.count("tags", None, None, &[]).unwrap();
        assert_eq!(result.matched, 3);
        assert!(result.total.is_none()); // no filter => no total
    }

    #[test]
    fn count_list_filtered() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "rust-lang", None, None).unwrap();
        store.push("tags", "focus", None, None).unwrap();
        store.push("tags", "rust-tools", None, None).unwrap();

        let result = store.count("tags", Some("rust"), None, &[]).unwrap();
        assert_eq!(result.matched, 2);
        assert_eq!(result.total, Some(3)); // 2 of 3 match
    }

    #[test]
    fn count_history_with_latest() {
        let (mut store, _dir) = setup_store(test_schema());
        let ts1 = DateTime::parse_from_rfc3339("2026-04-20T10:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts2 = DateTime::parse_from_rfc3339("2026-04-22T15:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("flavor_history", "bergamot", ts1, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "bergamot vanilla", ts2, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "lapsang", ts1, None, None)
            .unwrap();

        let result = store
            .count("flavor_history", Some("bergamot"), None, &[])
            .unwrap();
        assert_eq!(result.matched, 2);
        assert_eq!(result.total, Some(3)); // 2 of 3 match
        assert!(result.latest_ts.is_some());
        assert!(result.latest_ts.unwrap().contains("2026-04-22"));
    }

    #[test]
    fn count_empty() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.count("tags", None, None, &[]).unwrap();
        assert_eq!(result.matched, 0);
        assert!(result.total.is_none());
        assert!(result.latest_ts.is_none());
    }

    #[test]
    fn count_filtered_empty_total() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.count("tags", Some("rust"), None, &[]).unwrap();
        assert_eq!(result.matched, 0);
        assert_eq!(result.total, Some(0)); // 0 of 0
    }

    #[test]
    fn count_type_mismatch() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.count("warmth", None, None, &[]);
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    // -- ID assignment --

    #[test]
    fn history_ids_auto_increment() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("flavor_history", "a", None, None).unwrap();
        store.push("flavor_history", "b", None, None).unwrap();
        store.push("flavor_history", "c", None, None).unwrap();

        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                // Each push should get a unique index
                let indexes: Vec<u64> = entries.iter().map(|e| e.index).collect();
                assert_eq!(indexes.len(), 3);
                // All unique
                let mut unique = indexes.clone();
                unique.sort();
                unique.dedup();
                assert_eq!(unique.len(), 3);
            }
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn list_ids_auto_increment() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "a", None, None).unwrap();
        store.push("tags", "b", None, None).unwrap();
        store.push("tags", "c", None, None).unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items[0].index, 1);
                assert_eq!(items[1].index, 2);
                assert_eq!(items[2].index, 3);
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn list_ids_stable_after_remove() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "a", None, None).unwrap();
        store.push("tags", "b", None, None).unwrap();
        store.push("tags", "c", None, None).unwrap();

        // Remove "b"
        store
            .remove("tags", Some("b"), None::<&IdRef>, false)
            .unwrap();

        // Push "d" — should get id=4, not reuse id=2
        store.push("tags", "d", None, None).unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 3);
                assert_eq!(items[0].index, 1); // a
                assert_eq!(items[1].index, 3); // c
                assert_eq!(items[2].index, 4); // d
            }
            _ => panic!("Expected list"),
        }
    }

    // -- Backward compatibility --

    #[test]
    fn deserialize_old_bare_string_list() {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        // Write old-format data file with bare string list
        let old_data = r#"{
            "_schema": "test",
            "_updated": "2026-04-20T00:00:00Z",
            "tags": { "items": ["alpha", "beta", "gamma"] }
        }"#;
        fs::write(&data_path, old_data).unwrap();

        let store = KvStore::load(&schema_path, &data_path).unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 3);
                assert_eq!(items[0].value, "alpha");
                assert_eq!(items[1].value, "beta");
                assert_eq!(items[2].value, "gamma");
                // Should have auto-assigned indexes
                assert!(items[0].index > 0);
                assert!(items[1].index > items[0].index);
                // Timestamps should be empty (placeholder)
                assert!(items[0].ts.is_empty());
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn deserialize_old_history_without_id() {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        // Write old-format data file with history entries lacking id
        let old_data = r#"{
            "_schema": "test",
            "_updated": "2026-04-20T00:00:00Z",
            "flavor_history": {
                "entries": [
                    {"value": "bergamot", "ts": "2026-04-22T19:13:00Z"},
                    {"value": "lapsang", "ts": "2026-04-21T10:00:00Z"}
                ]
            }
        }"#;
        fs::write(&data_path, old_data).unwrap();

        let store = KvStore::load(&schema_path, &data_path).unwrap();

        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                assert_eq!(entries.len(), 2);
                assert_eq!(entries[0].value, "bergamot");
                // Should have auto-assigned indexes
                assert!(entries[0].index > 0);
                assert!(entries[1].index > 0);
                assert_ne!(entries[0].index, entries[1].index);
            }
            _ => panic!("Expected history"),
        }
    }

    // -- ListEntry timestamps --

    #[test]
    fn list_push_assigns_timestamp() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts = DateTime::parse_from_rfc3339("2026-04-22T10:30:00Z")
            .unwrap()
            .with_timezone(&Utc);
        store.push_with_ts("tags", "focus", ts, None, None).unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 1);
                assert!(items[0].ts.contains("2026-04-22"));
                assert_eq!(items[0].value, "focus");
            }
            _ => panic!("Expected list"),
        }
    }

    // -- format_value output --

    #[test]
    fn format_value_list_shows_ids_and_timestamps() {
        let (mut store, _dir) = setup_store(test_schema());
        let ts = DateTime::parse_from_rfc3339("2026-04-22T10:30:00Z")
            .unwrap()
            .with_timezone(&Utc);
        store.push_with_ts("tags", "focus", ts, None, None).unwrap();
        store.push_with_ts("tags", "rust", ts, None, None).unwrap();

        let output = format_value(store.get("tags").unwrap());
        // New format: "1 [kv-XXXX]: focus (ts)"
        assert!(output.contains("1 [kv-"));
        assert!(output.contains("]: focus"));
        assert!(output.contains("2 [kv-"));
        assert!(output.contains("]: rust"));
        assert!(output.contains("2026-04-22"));
    }

    #[test]
    fn format_value_history_shows_ids() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();

        let output = format_value(store.get("flavor_history").unwrap());
        // Should contain "N: bergamot (timestamp)"
        assert!(output.contains("bergamot"));
        assert!(output.contains(":"));
    }

    // -- compact dump with list timestamps --

    #[test]
    fn compact_dump_list_with_times() {
        let (mut store, _dir) = setup_store(test_schema());
        let ts = DateTime::parse_from_rfc3339("2026-04-22T14:15:00Z")
            .unwrap()
            .with_timezone(&Utc);
        store
            .push_with_ts("tags", "dsi-panel", ts, None, None)
            .unwrap();
        store
            .push_with_ts("tags", "anytype", ts, None, None)
            .unwrap();

        let compact = store.dump_compact();
        assert!(compact.contains("tags=[dsi-panel@14:15,anytype@14:15]"));
    }

    use chrono::Datelike as _;

    // -- Memory pointer operations --

    #[test]
    fn set_get_memory_on_history() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();

        // Initially no memory pointer
        assert_eq!(store.get_memory("flavor_history").unwrap(), None);

        // Set a memory pointer
        store
            .set_memory("flavor_history", Some("kn-abc123".to_string()))
            .unwrap();
        assert_eq!(
            store.get_memory("flavor_history").unwrap(),
            Some("kn-abc123")
        );

        // Verify it persists through save/reload
        store.data.schema_id = "test".to_string();
        store.save().unwrap();
        let data_str = fs::read_to_string(&store.data_path).unwrap();
        assert!(data_str.contains("kn-abc123"));
    }

    #[test]
    fn set_get_memory_on_list() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();

        store
            .set_memory("tags", Some("kn-def456".to_string()))
            .unwrap();
        assert_eq!(store.get_memory("tags").unwrap(), Some("kn-def456"));
    }

    #[test]
    fn set_get_memory_on_state() {
        let (mut store, _dir) = setup_store(test_schema());
        store.set("tensor", "0.5", Some("temperature")).unwrap();

        store
            .set_memory("tensor", Some("kn-789ghi".to_string()))
            .unwrap();
        assert_eq!(store.get_memory("tensor").unwrap(), Some("kn-789ghi"));
    }

    #[test]
    fn set_memory_on_counter_rejected() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set_memory("warmth", Some("kn-abc".to_string()));
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn set_memory_on_string_rejected() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.set_memory("current_mood", Some("kn-abc".to_string()));
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn get_memory_on_counter_rejected() {
        let (mut store, _dir) = setup_store(test_schema());
        store.inc("warmth", 1).unwrap();
        let result = store.get_memory("warmth");
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn clear_memory_with_empty_string() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();

        // Set then clear
        store
            .set_memory("flavor_history", Some("kn-abc123".to_string()))
            .unwrap();
        assert_eq!(
            store.get_memory("flavor_history").unwrap(),
            Some("kn-abc123")
        );

        // Clear with empty string
        store
            .set_memory("flavor_history", Some("".to_string()))
            .unwrap();
        assert_eq!(store.get_memory("flavor_history").unwrap(), None);
    }

    #[test]
    fn clear_memory_with_none() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();

        store
            .set_memory("tags", Some("kn-abc123".to_string()))
            .unwrap();
        store.set_memory("tags", None).unwrap();
        assert_eq!(store.get_memory("tags").unwrap(), None);
    }

    #[test]
    fn memory_not_serialized_when_none() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();
        store.data.schema_id = "test".to_string();
        store.save().unwrap();

        let data_str = fs::read_to_string(&store.data_path).unwrap();
        // "memory" should NOT appear in JSON when it's None
        assert!(!data_str.contains("\"memory\""));
    }

    #[test]
    fn backward_compat_old_data_without_memory() {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        // Old-format data file without memory field
        let old_data = r#"{
            "_schema": "test",
            "_updated": "2026-04-20T00:00:00Z",
            "flavor_history": {
                "entries": [
                    {"id": 1, "value": "bergamot", "ts": "2026-04-22T19:13:00Z"}
                ]
            },
            "tags": {
                "items": [
                    {"id": 1, "value": "focus", "ts": "2026-04-22T10:30:00Z"}
                ]
            },
            "tensor": {
                "fields": {"temperature": "0.55", "entropy": "0.35", "agency": "0.70"}
            }
        }"#;
        fs::write(&data_path, old_data).unwrap();

        let store = KvStore::load(&schema_path, &data_path).unwrap();

        // All should deserialize cleanly with no memory pointer
        assert_eq!(store.get_memory("flavor_history").unwrap(), None);
        assert_eq!(store.get_memory("tags").unwrap(), None);
        assert_eq!(store.get_memory("tensor").unwrap(), None);

        // Data should still be accessible
        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                assert_eq!(entries.len(), 1);
                assert_eq!(entries[0].value, "bergamot");
            }
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn backward_compat_data_with_memory() {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        // Data file WITH memory field
        let data = r#"{
            "_schema": "test",
            "_updated": "2026-04-20T00:00:00Z",
            "flavor_history": {
                "entries": [
                    {"id": 1, "value": "bergamot", "ts": "2026-04-22T19:13:00Z"}
                ],
                "memory": "kn-abc123"
            },
            "tags": {
                "items": [
                    {"id": 1, "value": "focus", "ts": "2026-04-22T10:30:00Z"}
                ],
                "memory": "kn-def456"
            }
        }"#;
        fs::write(&data_path, data).unwrap();

        let store = KvStore::load(&schema_path, &data_path).unwrap();

        assert_eq!(
            store.get_memory("flavor_history").unwrap(),
            Some("kn-abc123")
        );
        assert_eq!(store.get_memory("tags").unwrap(), Some("kn-def456"));
    }

    // -- Compact dump with memory pointers --

    #[test]
    fn compact_dump_with_memory_pointer() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts = DateTime::parse_from_rfc3339("2026-04-22T19:13:00Z")
            .unwrap()
            .with_timezone(&Utc);
        store
            .push_with_ts("flavor_history", "bergamot", ts, None, None)
            .unwrap();
        store
            .set_memory("flavor_history", Some("kn-def456".to_string()))
            .unwrap();

        store.set("tensor", "0.55", Some("temperature")).unwrap();
        store.set("tensor", "0.35", Some("entropy")).unwrap();
        store.set("tensor", "0.70", Some("agency")).unwrap();
        store
            .set_memory("tensor", Some("kn-789ghi".to_string()))
            .unwrap();

        let compact = store.dump_compact();
        assert!(compact.contains("flavor_history=[bergamot@19:13](kn-def456)"));
        assert!(compact.contains("tensor={0.55,0.35,0.70}(kn-789ghi)"));
    }

    #[test]
    fn compact_dump_without_memory_pointer() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts = DateTime::parse_from_rfc3339("2026-04-22T19:13:00Z")
            .unwrap()
            .with_timezone(&Utc);
        store
            .push_with_ts("flavor_history", "bergamot", ts, None, None)
            .unwrap();
        // No memory pointer set

        let compact = store.dump_compact();
        assert!(compact.contains("flavor_history=[bergamot@19:13]"));
        // Should NOT contain parenthetical
        assert!(!compact.contains("flavor_history=[bergamot@19:13]("));
    }

    #[test]
    fn set_memory_on_nonexistent_data_creates_default() {
        let (mut store, _dir) = setup_store(test_schema());

        // No data for flavor_history yet, but set memory should initialize it
        store
            .set_memory("flavor_history", Some("kn-abc123".to_string()))
            .unwrap();
        assert_eq!(
            store.get_memory("flavor_history").unwrap(),
            Some("kn-abc123")
        );

        // Should have created default empty history
        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => assert!(entries.is_empty()),
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn memory_survives_push() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();
        store
            .set_memory("flavor_history", Some("kn-abc123".to_string()))
            .unwrap();

        // Push another entry
        store.push("flavor_history", "lapsang", None, None).unwrap();

        // Memory pointer should still be set
        assert_eq!(
            store.get_memory("flavor_history").unwrap(),
            Some("kn-abc123")
        );
    }

    #[test]
    fn memory_survives_reset() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();
        store
            .set_memory("flavor_history", Some("kn-abc123".to_string()))
            .unwrap();

        // Reset clears data to default — memory pointer should be cleared too
        store.reset("flavor_history").unwrap();

        // After reset, memory is gone (default_value has memory: None)
        assert_eq!(store.get_memory("flavor_history").unwrap(), None);
    }

    // -----------------------------------------------------------------
    // Migration: ~/.crewu/kv -> $MX_HOME/kv (decision 1)
    // -----------------------------------------------------------------

    #[test]
    fn kv_path_env_override_wins() {
        let (path, warn) = resolve_kv_path_with(
            Some("/explicit/{agent}.toml"),
            "smith",
            std::path::PathBuf::from("/new/smith.toml"),
            Some(std::path::PathBuf::from("/legacy/smith.toml")),
        );
        assert_eq!(path, std::path::PathBuf::from("/explicit/smith.toml"));
        assert!(!warn);
    }

    #[test]
    fn kv_path_uses_new_default_when_present() {
        let dir = tempfile::tempdir().unwrap();
        let new_path = dir.path().join("new.toml");
        std::fs::write(&new_path, "").unwrap();
        let legacy = dir.path().join("legacy.toml");
        std::fs::write(&legacy, "").unwrap();

        let (path, warn) = resolve_kv_path_with(None, "smith", new_path.clone(), Some(legacy));
        assert_eq!(path, new_path);
        assert!(!warn);
    }

    #[test]
    fn kv_path_falls_back_to_legacy_with_warning() {
        let dir = tempfile::tempdir().unwrap();
        let new_path = dir.path().join("missing.toml"); // doesn't exist
        let legacy = dir.path().join("legacy.toml");
        std::fs::write(&legacy, "").unwrap();

        let (path, warn) = resolve_kv_path_with(None, "smith", new_path, Some(legacy.clone()));
        assert_eq!(path, legacy);
        assert!(warn, "warning MUST fire on legacy fallback");
    }

    #[test]
    fn kv_path_returns_new_default_when_neither_exists() {
        let dir = tempfile::tempdir().unwrap();
        let new_path = dir.path().join("missing.toml");
        let legacy = dir.path().join("also-missing.toml");

        let (path, warn) = resolve_kv_path_with(None, "smith", new_path.clone(), Some(legacy));
        assert_eq!(path, new_path);
        assert!(!warn);
    }

    #[test]
    fn kv_path_empty_env_treated_as_unset() {
        let dir = tempfile::tempdir().unwrap();
        let new_path = dir.path().join("new.toml");
        std::fs::write(&new_path, "").unwrap();
        let (path, warn) = resolve_kv_path_with(Some(""), "smith", new_path.clone(), None);
        assert_eq!(path, new_path);
        assert!(!warn);
    }

    // -----------------------------------------------------------------
    // should_emit_legacy_kv_warning -- dedupe gate (Critical 2 fix)
    // -----------------------------------------------------------------

    #[test]
    fn legacy_warning_silent_when_no_fallback() {
        let gate = std::sync::OnceLock::new();
        assert!(!should_emit_legacy_kv_warning(false, false, &gate));
        // gate must remain unset so a later real fallback still warns
        assert!(gate.get().is_none());
    }

    #[test]
    fn legacy_warning_fires_once_when_only_schema_warns() {
        let gate = std::sync::OnceLock::new();
        assert!(should_emit_legacy_kv_warning(true, false, &gate));
        assert!(!should_emit_legacy_kv_warning(true, false, &gate));
    }

    #[test]
    fn legacy_warning_fires_once_when_only_data_warns() {
        let gate = std::sync::OnceLock::new();
        assert!(should_emit_legacy_kv_warning(false, true, &gate));
        assert!(!should_emit_legacy_kv_warning(false, true, &gate));
    }

    #[test]
    fn legacy_warning_fires_once_when_both_warn() {
        // Models the Critical-2 scenario: both schema and data legacy-fallback
        // in a single `from_env` call -- user must see ONE warning, not two.
        let gate = std::sync::OnceLock::new();
        assert!(should_emit_legacy_kv_warning(true, true, &gate));
        // Subsequent calls (e.g. another resolver in the same process) stay quiet.
        assert!(!should_emit_legacy_kv_warning(true, true, &gate));
        assert!(!should_emit_legacy_kv_warning(false, true, &gate));
        assert!(!should_emit_legacy_kv_warning(true, false, &gate));
    }

    // -----------------------------------------------------------------
    // Production resolvers -- exercise the actual code path that ships
    // (Critical 1 fix). These touch env vars so they share a mutex.
    // -----------------------------------------------------------------

    use std::sync::Mutex;
    static ENV_LOCK: Mutex<()> = Mutex::new(());

    /// RAII guard: sets an env var on construction, restores on drop.
    struct EnvGuard {
        key: &'static str,
        prior: Option<String>,
    }

    impl EnvGuard {
        fn set(key: &'static str, val: &str) -> Self {
            let prior = std::env::var(key).ok();
            // SAFETY: tests serialize on ENV_LOCK before constructing guards.
            unsafe {
                std::env::set_var(key, val);
            }
            Self { key, prior }
        }

        fn unset(key: &'static str) -> Self {
            let prior = std::env::var(key).ok();
            // SAFETY: see above.
            unsafe {
                std::env::remove_var(key);
            }
            Self { key, prior }
        }
    }

    impl Drop for EnvGuard {
        fn drop(&mut self) {
            // SAFETY: tests serialize on ENV_LOCK; this runs while guard is held.
            unsafe {
                match &self.prior {
                    Some(v) => std::env::set_var(self.key, v),
                    None => std::env::remove_var(self.key),
                }
            }
        }
    }

    #[test]
    fn production_resolve_schema_path_honors_env_with_agent_substitution() {
        let _lock = ENV_LOCK.lock().unwrap();
        let _g = EnvGuard::set("MX_KV_SCHEMA", "/tmp/explicit/{agent}.toml");
        let (path, warn) = KvStore::resolve_schema_path("smith");
        assert_eq!(path, std::path::PathBuf::from("/tmp/explicit/smith.toml"));
        assert!(!warn);
    }

    #[test]
    fn production_resolve_data_path_honors_env_with_agent_substitution() {
        let _lock = ENV_LOCK.lock().unwrap();
        let _g = EnvGuard::set("MX_KV_DATA", "/tmp/explicit/{agent}.json");
        let (path, warn) = KvStore::resolve_data_path("smith");
        assert_eq!(path, std::path::PathBuf::from("/tmp/explicit/smith.json"));
        assert!(!warn);
    }

    #[test]
    fn production_resolve_schema_path_empty_env_is_unset() {
        let _lock = ENV_LOCK.lock().unwrap();
        let _g = EnvGuard::set("MX_KV_SCHEMA", "");
        // Empty env -> resolver should NOT return PathBuf::from("") -- it
        // should fall through to the default-derived path. The exact path
        // depends on which file (if any) exists on the test host -- so we
        // assert against the canonical helpers, not string literals.
        let (path, _warn) = KvStore::resolve_schema_path("smith");
        assert_ne!(path, std::path::PathBuf::from(""));
        let new_default = crate::paths::kv_schema_path("smith");
        let legacy = crate::paths::legacy_crewu_kv_schema_path("smith");
        assert!(
            path == new_default || legacy.as_ref() == Some(&path),
            "expected one of the derived paths, got: {}",
            path.display()
        );
    }

    #[test]
    fn production_resolve_data_path_empty_env_is_unset() {
        let _lock = ENV_LOCK.lock().unwrap();
        let _g = EnvGuard::set("MX_KV_DATA", "");
        let (path, _warn) = KvStore::resolve_data_path("smith");
        assert_ne!(path, std::path::PathBuf::from(""));
        let new_default = crate::paths::kv_data_path("smith");
        let legacy = crate::paths::legacy_crewu_kv_data_path("smith");
        assert!(
            path == new_default || legacy.as_ref() == Some(&path),
            "expected one of the derived paths, got: {}",
            path.display()
        );
    }

    #[test]
    fn production_resolve_schema_path_returns_default_when_unset() {
        let _lock = ENV_LOCK.lock().unwrap();
        let _g = EnvGuard::unset("MX_KV_SCHEMA");
        let (path, _warn) = KvStore::resolve_schema_path("smith");
        // Either the new $MX_HOME default OR the legacy fallback (if it
        // happens to exist on the test host). Both are acceptable -- we
        // assert against the canonical helpers so this test never hardcodes
        // a string literal that the path-alignment grep guards forbid.
        let new_default = crate::paths::kv_schema_path("smith");
        let legacy = crate::paths::legacy_crewu_kv_schema_path("smith");
        assert!(
            path == new_default || legacy.as_ref() == Some(&path),
            "unexpected default path: {}",
            path.display()
        );
    }

    // -----------------------------------------------------------------
    // Time-range parsing
    // -----------------------------------------------------------------

    #[test]
    fn parse_day_valid() {
        let range = parse_day("2026-04-25").unwrap();
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-04-25");
        assert_eq!(range.to.format("%Y-%m-%d").to_string(), "2026-04-26");
    }

    #[test]
    fn parse_day_invalid_format() {
        assert!(parse_day("04-25-2026").is_err());
        assert!(parse_day("2026/04/25").is_err());
        assert!(parse_day("not-a-date").is_err());
    }

    #[test]
    fn parse_month_valid() {
        let range = parse_month("2026-04").unwrap();
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-04-01");
        assert_eq!(range.to.format("%Y-%m-%d").to_string(), "2026-05-01");
    }

    #[test]
    fn parse_month_december_rollover() {
        let range = parse_month("2026-12").unwrap();
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-12-01");
        assert_eq!(range.to.format("%Y-%m-%d").to_string(), "2027-01-01");
    }

    #[test]
    fn parse_month_invalid_format() {
        assert!(parse_month("2026").is_err());
        assert!(parse_month("2026-13").is_err());
        assert!(parse_month("2026-00").is_err());
        assert!(parse_month("not-a-month").is_err());
    }

    #[test]
    fn parse_week_valid() {
        let range = parse_week("2026-W17").unwrap();
        // ISO week 17 of 2026 starts on Monday 2026-04-20
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-04-20");
        assert_eq!(range.to.format("%Y-%m-%d").to_string(), "2026-04-27");
    }

    #[test]
    fn parse_week_1() {
        let range = parse_week("2026-W01").unwrap();
        // ISO week 1 of 2026 starts on Monday 2025-12-29
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2025-12-29");
        let diff = range.to - range.from;
        assert_eq!(diff.num_days(), 7);
    }

    #[test]
    fn parse_week_invalid_format() {
        assert!(parse_week("2026-17").is_err());
        assert!(parse_week("2026-W00").is_err());
        assert!(parse_week("not-a-week").is_err());
    }

    #[test]
    fn parse_date_range_both_provided() {
        let range = parse_date_range(Some("2026-04-01"), Some("2026-04-15")).unwrap();
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-04-01");
        // to is inclusive of the end date, so upper bound is start of next day
        assert_eq!(range.to.format("%Y-%m-%d").to_string(), "2026-04-16");
    }

    #[test]
    fn parse_date_range_from_only() {
        let range = parse_date_range(Some("2026-04-01"), None).unwrap();
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-04-01");
        // to defaults to now, so it should be roughly today
        let diff = Utc::now() - range.to;
        assert!(diff.num_seconds().abs() < 5);
    }

    #[test]
    fn parse_date_range_to_only() {
        let range = parse_date_range(None, Some("2026-04-15")).unwrap();
        assert_eq!(range.from, DateTime::UNIX_EPOCH);
        assert_eq!(range.to.format("%Y-%m-%d").to_string(), "2026-04-16");
    }

    #[test]
    fn parse_date_range_from_after_to_errors() {
        let result = parse_date_range(Some("2026-04-15"), Some("2026-04-01"));
        assert!(result.is_err());
    }

    #[test]
    fn resolve_time_range_no_flags() {
        let args = TimeRangeArgs::default();
        assert!(resolve_time_range(&args).unwrap().is_none());
    }

    #[test]
    fn resolve_time_range_day() {
        let args = TimeRangeArgs {
            day: Some("2026-04-25".to_string()),
            ..Default::default()
        };
        let range = resolve_time_range(&args).unwrap().unwrap();
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-04-25");
    }

    #[test]
    fn resolve_time_range_month() {
        let args = TimeRangeArgs {
            month: Some("2026-04".to_string()),
            ..Default::default()
        };
        let range = resolve_time_range(&args).unwrap().unwrap();
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-04-01");
    }

    #[test]
    fn resolve_time_range_week() {
        let args = TimeRangeArgs {
            week: Some("2026-W17".to_string()),
            ..Default::default()
        };
        let range = resolve_time_range(&args).unwrap().unwrap();
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-04-20");
    }

    #[test]
    fn resolve_time_range_from_to() {
        let args = TimeRangeArgs {
            range_from: Some("2026-04-01".to_string()),
            range_to: Some("2026-04-15".to_string()),
            ..Default::default()
        };
        let range = resolve_time_range(&args).unwrap().unwrap();
        assert_eq!(range.from.format("%Y-%m-%d").to_string(), "2026-04-01");
        assert_eq!(range.to.format("%Y-%m-%d").to_string(), "2026-04-16");
    }

    // -----------------------------------------------------------------
    // ts_in_range
    // -----------------------------------------------------------------

    #[test]
    fn ts_in_range_inside() {
        let range = parse_day("2026-04-25").unwrap();
        assert!(ts_in_range("2026-04-25T12:00:00+00:00", &range));
    }

    #[test]
    fn ts_in_range_at_lower_boundary() {
        let range = parse_day("2026-04-25").unwrap();
        assert!(ts_in_range("2026-04-25T00:00:00+00:00", &range));
    }

    #[test]
    fn ts_in_range_at_upper_boundary_excluded() {
        let range = parse_day("2026-04-25").unwrap();
        // Upper bound is exclusive, so midnight of the next day is out
        assert!(!ts_in_range("2026-04-26T00:00:00+00:00", &range));
    }

    #[test]
    fn ts_in_range_outside() {
        let range = parse_day("2026-04-25").unwrap();
        assert!(!ts_in_range("2026-04-24T23:59:59+00:00", &range));
        assert!(!ts_in_range("2026-04-26T00:00:01+00:00", &range));
    }

    #[test]
    fn ts_in_range_empty_ts() {
        let range = parse_day("2026-04-25").unwrap();
        assert!(!ts_in_range("", &range));
    }

    #[test]
    fn ts_in_range_invalid_ts() {
        let range = parse_day("2026-04-25").unwrap();
        assert!(!ts_in_range("not-a-timestamp", &range));
    }

    // -----------------------------------------------------------------
    // Time-range integration with last/search/count
    // -----------------------------------------------------------------

    #[test]
    fn last_with_time_range_filters_history() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts_in = DateTime::parse_from_rfc3339("2026-04-25T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts_out = DateTime::parse_from_rfc3339("2026-04-24T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("flavor_history", "inside", ts_in, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "outside", ts_out, None, None)
            .unwrap();

        let range = parse_day("2026-04-25").unwrap();
        let results = store.last("flavor_history", 10, Some(&range), &[]).unwrap();
        assert_eq!(results.len(), 1);
        assert_eq!(results[0].value, "inside");
    }

    #[test]
    fn last_with_time_range_composes_with_count() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts = DateTime::parse_from_rfc3339("2026-04-25T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("flavor_history", "a", ts, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "b", ts, None, None)
            .unwrap();

        let range = parse_day("2026-04-25").unwrap();
        // Both entries match the range, but count=1 limits output
        let results = store.last("flavor_history", 1, Some(&range), &[]).unwrap();
        assert_eq!(results.len(), 1);
    }

    #[test]
    fn search_with_time_range_filters() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts_in = DateTime::parse_from_rfc3339("2026-04-25T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts_out = DateTime::parse_from_rfc3339("2026-04-20T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("tags", "rust-in", ts_in, None, None)
            .unwrap();
        store
            .push_with_ts("tags", "rust-out", ts_out, None, None)
            .unwrap();

        let range = parse_day("2026-04-25").unwrap();
        let hits = store
            .search("tags", Some("rust"), Some(&range), &[])
            .unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "rust-in");
    }

    #[test]
    fn last_with_time_range_filters_list() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts_in = DateTime::parse_from_rfc3339("2026-04-25T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts_out = DateTime::parse_from_rfc3339("2026-04-24T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("tags", "inside", ts_in, None, None)
            .unwrap();
        store
            .push_with_ts("tags", "outside", ts_out, None, None)
            .unwrap();
        store
            .push_with_ts("tags", "also-inside", ts_in, None, None)
            .unwrap();

        let range = parse_day("2026-04-25").unwrap();
        let results = store.last("tags", 10, Some(&range), &[]).unwrap();
        assert_eq!(results.len(), 2);
        assert_eq!(results[0].value, "inside");
        assert_eq!(results[1].value, "also-inside");

        // count limit applies after time filter
        let results = store.last("tags", 1, Some(&range), &[]).unwrap();
        assert_eq!(results.len(), 1);
        assert_eq!(results[0].value, "also-inside");
    }

    #[test]
    fn search_with_time_range_filters_history() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts_in = DateTime::parse_from_rfc3339("2026-04-25T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts_out = DateTime::parse_from_rfc3339("2026-04-20T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("flavor_history", "bergamot-in", ts_in, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "bergamot-out", ts_out, None, None)
            .unwrap();

        let range = parse_day("2026-04-25").unwrap();
        let hits = store
            .search("flavor_history", Some("bergamot"), Some(&range), &[])
            .unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "bergamot-in");
    }

    #[test]
    fn count_with_time_range_filters() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts_in = DateTime::parse_from_rfc3339("2026-04-25T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts_out = DateTime::parse_from_rfc3339("2026-04-20T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("flavor_history", "bergamot", ts_in, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "lapsang", ts_in, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "bergamot earl", ts_out, None, None)
            .unwrap();

        let range = parse_day("2026-04-25").unwrap();
        // Count all in range (no value filter)
        let result = store
            .count("flavor_history", None, Some(&range), &[])
            .unwrap();
        assert_eq!(result.matched, 2);

        // Count with value filter + time range
        let result = store
            .count("flavor_history", Some("bergamot"), Some(&range), &[])
            .unwrap();
        assert_eq!(result.matched, 1);
        assert_eq!(result.total, Some(2)); // 1 of 2 in-range entries match
    }

    #[test]
    fn count_with_month_range() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts_apr = DateTime::parse_from_rfc3339("2026-04-15T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts_may = DateTime::parse_from_rfc3339("2026-05-02T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("flavor_history", "april", ts_apr, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "may", ts_may, None, None)
            .unwrap();

        let range = parse_month("2026-04").unwrap();
        let result = store
            .count("flavor_history", None, Some(&range), &[])
            .unwrap();
        assert_eq!(result.matched, 1);
    }

    // -- Random sampling --

    #[test]
    fn random_on_empty_returns_empty() {
        let (store, _dir) = setup_store(test_schema());
        let results = store.random("flavor_history", 5, None, &[]).unwrap();
        assert!(results.is_empty());
    }

    #[test]
    fn random_returns_requested_count() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();
        store.push("tags", "gamma", None, None).unwrap();

        let results = store.random("tags", 2, None, &[]).unwrap();
        assert_eq!(results.len(), 2);
    }

    #[test]
    fn random_returns_all_when_count_exceeds_available() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();

        let results = store.random("tags", 10, None, &[]).unwrap();
        assert_eq!(results.len(), 2);
    }

    #[test]
    fn random_type_mismatch_on_counter() {
        let (store, _dir) = setup_store(test_schema());
        let result = store.random("warmth", 1, None, &[]);
        assert!(result.is_err());
        match result.unwrap_err() {
            KvError::TypeMismatch { key, .. } => assert_eq!(key, "warmth"),
            other => panic!("Expected TypeMismatch, got {:?}", other),
        }
    }

    #[test]
    fn random_works_on_history() {
        let (mut store, _dir) = setup_store(test_schema());
        let ts = DateTime::parse_from_rfc3339("2026-04-25T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("flavor_history", "bergamot", ts, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "vanilla", ts, None, None)
            .unwrap();

        let results = store.random("flavor_history", 1, None, &[]).unwrap();
        assert_eq!(results.len(), 1);
        // Result must be one of the pushed values
        assert!(results[0].value == "bergamot" || results[0].value == "vanilla");
    }

    #[test]
    fn random_with_time_range_filters() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts_in = DateTime::parse_from_rfc3339("2026-04-25T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts_out = DateTime::parse_from_rfc3339("2026-04-20T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("tags", "inside", ts_in, None, None)
            .unwrap();
        store
            .push_with_ts("tags", "outside", ts_out, None, None)
            .unwrap();

        let range = parse_day("2026-04-25").unwrap();
        let results = store.random("tags", 10, Some(&range), &[]).unwrap();
        assert_eq!(results.len(), 1);
        assert_eq!(results[0].value, "inside");
    }

    #[test]
    fn random_with_time_range_filters_history() {
        let (mut store, _dir) = setup_store(test_schema());

        let ts_in = DateTime::parse_from_rfc3339("2026-04-25T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts_out = DateTime::parse_from_rfc3339("2026-04-24T12:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("flavor_history", "inside", ts_in, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "outside", ts_out, None, None)
            .unwrap();

        let range = parse_day("2026-04-25").unwrap();
        let results = store
            .random("flavor_history", 10, Some(&range), &[])
            .unwrap();
        assert_eq!(results.len(), 1);
        assert_eq!(results[0].value, "inside");
    }

    // -- --since resolver --

    #[test]
    fn resolve_time_range_since_relative() {
        let args = TimeRangeArgs {
            since: Some("1h".to_string()),
            ..Default::default()
        };
        let range = resolve_time_range(&args).unwrap();
        assert!(range.is_some());
        let range = range.unwrap();
        // "since 1h" means from ~1 hour ago to now
        let one_hour_ago = Utc::now() - chrono::Duration::hours(1);
        // Allow a few seconds of drift
        assert!((range.from - one_hour_ago).num_seconds().abs() < 5);
        assert!((range.to - Utc::now()).num_seconds().abs() < 5);
    }

    #[test]
    fn resolve_time_range_since_days() {
        let args = TimeRangeArgs {
            since: Some("30d".to_string()),
            ..Default::default()
        };
        let range = resolve_time_range(&args).unwrap();
        assert!(range.is_some());
        let range = range.unwrap();
        let thirty_days_ago = Utc::now() - chrono::Duration::days(30);
        assert!((range.from - thirty_days_ago).num_seconds().abs() < 5);
    }

    // -----------------------------------------------------------------------
    // Structured data tests
    // -----------------------------------------------------------------------

    #[test]
    fn push_with_data_stores_data() {
        let (mut store, _dir) = setup_store(test_schema());
        let data = serde_json::json!({"status": "active", "tags": ["rust", "kv"]});
        store
            .push("tags", "my-item", Some(data.clone()), None)
            .unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 1);
                assert_eq!(items[0].value, "my-item");
                assert_eq!(items[0].data, Some(data));
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn push_without_data_stores_none() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "bare-item", None, None).unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 1);
                assert!(items[0].data.is_none());
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn push_history_with_data() {
        let (mut store, _dir) = setup_store(test_schema());
        let data = serde_json::json!({"mood": "focused"});
        store
            .push("flavor_history", "bergamot", Some(data.clone()), None)
            .unwrap();

        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                assert_eq!(entries[0].data, Some(data));
            }
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn data_round_trip_through_save() {
        let (mut store, _dir) = setup_store(test_schema());
        let data = serde_json::json!({"priority": 1, "tags": ["a", "b"]});
        store
            .push("tags", "item", Some(data.clone()), None)
            .unwrap();
        store.data.schema_id = "test".to_string();
        store.save().unwrap();

        // Reload
        let data_str = fs::read_to_string(&store.data_path).unwrap();
        let reloaded: DataFile = serde_json::from_str(&data_str).unwrap();
        match &reloaded.entries["tags"] {
            DataValue::List { items, .. } => {
                assert_eq!(items[0].data, Some(data));
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn backward_compat_missing_data_field() {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        // Write data without any "data" fields
        let old_data = r#"{
            "_schema": "test",
            "_updated": "2026-04-20T00:00:00Z",
            "tags": { "items": [{"id": 1, "value": "alpha", "ts": "2026-04-20T00:00:00Z"}] },
            "flavor_history": { "entries": [{"id": 1, "value": "bergamot", "ts": "2026-04-20T00:00:00Z"}] }
        }"#;
        fs::write(&data_path, old_data).unwrap();

        let store = KvStore::load(&schema_path, &data_path).unwrap();

        // List entry should have data=None
        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert!(items[0].data.is_none());
            }
            _ => panic!("Expected list"),
        }

        // History entry should have data=None
        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                assert!(entries[0].data.is_none());
            }
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn backward_compat_bare_string_list_gets_none_data() {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        let old_data = r#"{
            "_schema": "test",
            "_updated": "2026-04-20T00:00:00Z",
            "tags": { "items": ["alpha", "beta"] }
        }"#;
        fs::write(&data_path, old_data).unwrap();

        let store = KvStore::load(&schema_path, &data_path).unwrap();
        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 2);
                assert!(items[0].data.is_none());
                assert!(items[1].data.is_none());
            }
            _ => panic!("Expected list"),
        }
    }

    // -- where_matches unit tests --

    #[test]
    fn where_matches_empty_clauses() {
        assert!(where_matches(&None, &[]));
        assert!(where_matches(&Some(serde_json::json!({"a": "b"})), &[]));
    }

    #[test]
    fn where_matches_none_data_nonempty_clauses() {
        let clauses = vec![("status".to_string(), "active".to_string())];
        assert!(!where_matches(&None, &clauses));
    }

    #[test]
    fn where_matches_exact_string() {
        let data = Some(serde_json::json!({"status": "active", "priority": "high"}));
        let clauses = vec![("status".to_string(), "active".to_string())];
        assert!(where_matches(&data, &clauses));
    }

    #[test]
    fn where_matches_exact_string_no_match() {
        let data = Some(serde_json::json!({"status": "closed"}));
        let clauses = vec![("status".to_string(), "active".to_string())];
        assert!(!where_matches(&data, &clauses));
    }

    #[test]
    fn where_matches_array_contains() {
        let data = Some(serde_json::json!({"tags": ["rust", "cli", "kv"]}));
        let clauses = vec![("tags".to_string(), "rust".to_string())];
        assert!(where_matches(&data, &clauses));
    }

    #[test]
    fn where_matches_array_not_contains() {
        let data = Some(serde_json::json!({"tags": ["rust", "cli"]}));
        let clauses = vec![("tags".to_string(), "python".to_string())];
        assert!(!where_matches(&data, &clauses));
    }

    #[test]
    fn where_matches_multiple_clauses_and_logic() {
        let data = Some(serde_json::json!({"status": "active", "priority": "high"}));

        // Both match
        let clauses = vec![
            ("status".to_string(), "active".to_string()),
            ("priority".to_string(), "high".to_string()),
        ];
        assert!(where_matches(&data, &clauses));

        // One fails
        let clauses = vec![
            ("status".to_string(), "active".to_string()),
            ("priority".to_string(), "low".to_string()),
        ];
        assert!(!where_matches(&data, &clauses));
    }

    #[test]
    fn where_matches_missing_key() {
        let data = Some(serde_json::json!({"status": "active"}));
        let clauses = vec![("nonexistent".to_string(), "value".to_string())];
        assert!(!where_matches(&data, &clauses));
    }

    #[test]
    fn where_matches_number_as_string() {
        let data = Some(serde_json::json!({"count": 42}));
        let clauses = vec![("count".to_string(), "42".to_string())];
        assert!(where_matches(&data, &clauses));
    }

    #[test]
    fn where_matches_bool_as_string() {
        let data = Some(serde_json::json!({"active": true}));
        let clauses = vec![("active".to_string(), "true".to_string())];
        assert!(where_matches(&data, &clauses));
    }

    // -- search with where_clauses --

    #[test]
    fn search_with_where_exact_match() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push(
                "tags",
                "task-1",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "task-2",
                Some(serde_json::json!({"status": "closed"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "task-3",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();

        let clauses = vec![("status".to_string(), "active".to_string())];
        let hits = store.search("tags", None, None, &clauses).unwrap();
        assert_eq!(hits.len(), 2);
        assert_eq!(hits[0].value, "task-1");
        assert_eq!(hits[1].value, "task-3");
    }

    #[test]
    fn search_with_where_array_contains() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push(
                "tags",
                "item-a",
                Some(serde_json::json!({"labels": ["bug", "urgent"]})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "item-b",
                Some(serde_json::json!({"labels": ["feature"]})),
                None,
            )
            .unwrap();

        let clauses = vec![("labels".to_string(), "bug".to_string())];
        let hits = store.search("tags", None, None, &clauses).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "item-a");
    }

    #[test]
    fn search_with_where_excludes_entries_without_data() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "no-data", None, None).unwrap();
        store
            .push(
                "tags",
                "has-data",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();

        let clauses = vec![("status".to_string(), "active".to_string())];
        let hits = store.search("tags", None, None, &clauses).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "has-data");
    }

    #[test]
    fn search_with_query_and_where() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push(
                "tags",
                "rust-fix",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "rust-feature",
                Some(serde_json::json!({"status": "closed"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "python-fix",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();

        let clauses = vec![("status".to_string(), "active".to_string())];
        // Text query "rust" AND where status=active
        let hits = store.search("tags", Some("rust"), None, &clauses).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "rust-fix");
    }

    #[test]
    fn search_with_only_where_no_query() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("tags", "a", Some(serde_json::json!({"type": "bug"})), None)
            .unwrap();
        store
            .push(
                "tags",
                "b",
                Some(serde_json::json!({"type": "feature"})),
                None,
            )
            .unwrap();

        let clauses = vec![("type".to_string(), "bug".to_string())];
        let hits = store.search("tags", None, None, &clauses).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "a");
    }

    #[test]
    fn search_multiple_where_clauses_and() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push(
                "tags",
                "match-both",
                Some(serde_json::json!({"status": "active", "priority": "high"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "match-one",
                Some(serde_json::json!({"status": "active", "priority": "low"})),
                None,
            )
            .unwrap();

        let clauses = vec![
            ("status".to_string(), "active".to_string()),
            ("priority".to_string(), "high".to_string()),
        ];
        let hits = store.search("tags", None, None, &clauses).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "match-both");
    }

    // -- last with where_clauses --

    #[test]
    fn last_with_where() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push(
                "tags",
                "a",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "b",
                Some(serde_json::json!({"status": "closed"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "c",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();

        let clauses = vec![("status".to_string(), "active".to_string())];
        let items = store.last("tags", 10, None, &clauses).unwrap();
        assert_eq!(items.len(), 2);
        assert_eq!(items[0].value, "a");
        assert_eq!(items[1].value, "c");
    }

    // -- random with where_clauses --

    #[test]
    fn random_with_where() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push(
                "tags",
                "active-1",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "closed-1",
                Some(serde_json::json!({"status": "closed"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "active-2",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();

        let clauses = vec![("status".to_string(), "active".to_string())];
        let items = store.random("tags", 10, None, &clauses).unwrap();
        assert_eq!(items.len(), 2);
        // All returned items should contain "active-"
        for item in &items {
            assert!(
                item.value.contains("active-"),
                "Expected active item, got: {}",
                item.value
            );
        }
    }

    // -- count with where_clauses --

    #[test]
    fn count_with_where() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push(
                "tags",
                "a",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "b",
                Some(serde_json::json!({"status": "closed"})),
                None,
            )
            .unwrap();
        store
            .push(
                "tags",
                "c",
                Some(serde_json::json!({"status": "active"})),
                None,
            )
            .unwrap();

        let clauses = vec![("status".to_string(), "active".to_string())];
        let result = store.count("tags", None, None, &clauses).unwrap();
        assert_eq!(result.matched, 2);
        assert_eq!(result.total, Some(3)); // where_clauses count as filtering
    }

    // -- search hits carry data field --

    #[test]
    fn search_hits_carry_data() {
        let (mut store, _dir) = setup_store(test_schema());
        let data = serde_json::json!({"status": "active"});
        store
            .push("tags", "item", Some(data.clone()), None)
            .unwrap();

        let hits = store.search("tags", Some("item"), None, &[]).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].data, Some(data));
    }

    #[test]
    fn get_entries_by_id_carries_data() {
        let (mut store, _dir) = setup_store(test_schema());
        let data = serde_json::json!({"priority": "high"});
        store
            .push("tags", "item", Some(data.clone()), None)
            .unwrap();

        let hits = store.get_entries_by_id("tags", &[IdRef::Index(1)]).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].data, Some(data));
    }

    // -- format_data_suffix --

    #[test]
    fn format_data_suffix_none() {
        assert_eq!(format_data_suffix(&None), "");
    }

    #[test]
    fn format_data_suffix_some() {
        let data = Some(serde_json::json!({"status": "active"}));
        let suffix = format_data_suffix(&data);
        assert!(suffix.starts_with(' '));
        // Should be compact JSON (no pretty printing)
        assert!(!suffix.contains('\n'));
        assert!(suffix.contains("\"status\""));
        assert!(suffix.contains("\"active\""));
    }

    // -- format_value includes data --

    #[test]
    fn format_value_includes_data() {
        let (mut store, _dir) = setup_store(test_schema());
        let data = serde_json::json!({"k": "v"});
        store.push("tags", "item", Some(data), None).unwrap();

        let formatted = format_value(store.get("tags").unwrap());
        assert!(formatted.contains("{\"k\":\"v\"}"));
    }

    #[test]
    fn format_value_no_data_unchanged() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "plain-item", None, None).unwrap();

        let formatted = format_value(store.get("tags").unwrap());
        assert!(formatted.contains("plain-item"));
        // Should NOT have trailing JSON
        assert!(!formatted.contains('{'));
    }

    // -- last output includes data suffix --

    #[test]
    fn last_output_includes_data() {
        let (mut store, _dir) = setup_store(test_schema());
        let data = serde_json::json!({"x": 1});
        store.push("tags", "item", Some(data), None).unwrap();

        let items = store.last("tags", 1, None, &[]).unwrap();
        assert_eq!(items.len(), 1);
        assert_eq!(items[0].data, Some(serde_json::json!({"x": 1})));
    }

    // -- ID generation tests --

    #[test]
    fn push_returns_push_result_with_index_and_id() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("tags", "alpha", None, None).unwrap();
        assert_eq!(result.index, 1);
        assert!(!result.id.is_empty());
        // ID should be 5-6 base58 chars
        assert!(result.id.len() >= 4);
        assert!(result.id.len() <= 8);
    }

    #[test]
    fn id_is_stable_same_inputs() {
        // generate_entry_id is deterministic
        let h1 = generate_entry_id("tags", "2026-05-08T00:00:00+00:00", 1);
        let h2 = generate_entry_id("tags", "2026-05-08T00:00:00+00:00", 1);
        assert_eq!(h1, h2);
    }

    #[test]
    fn id_is_unique_different_inputs() {
        let h1 = generate_entry_id("tags", "2026-05-08T00:00:00+00:00", 1);
        let h2 = generate_entry_id("tags", "2026-05-08T00:00:00+00:00", 2);
        let h3 = generate_entry_id("other", "2026-05-08T00:00:00+00:00", 1);
        assert_ne!(h1, h2);
        assert_ne!(h1, h3);
    }

    #[test]
    fn backfill_old_entries_get_ids_on_load() {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        // Write old-format data with no hash field (on disk "id" is the numeric index)
        let old_data = r#"{
            "_schema": "test",
            "_updated": "2026-05-08T00:00:00+00:00",
            "tags": {
                "items": [
                    {"id": 1, "value": "alpha", "ts": "2026-05-08T00:00:00+00:00"},
                    {"id": 2, "value": "beta", "ts": "2026-05-08T00:01:00+00:00"}
                ]
            }
        }"#;
        fs::write(&data_path, old_data).unwrap();

        let store = KvStore::load(&schema_path, &data_path).unwrap();
        match store.data.entries.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert!(!items[0].id.is_empty(), "id should be back-filled");
                assert!(!items[1].id.is_empty(), "id should be back-filled");
                // IDs should differ
                assert_ne!(items[0].id, items[1].id);
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn get_by_numeric_index_still_works() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("tags", "alpha", None, None).unwrap();

        let hits = store
            .get_entries_by_id("tags", &[IdRef::Index(result.index)])
            .unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "alpha");
    }

    #[test]
    fn get_by_id_works() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("tags", "alpha", None, None).unwrap();

        let hits = store
            .get_entries_by_id("tags", &[IdRef::Id(result.id.clone())])
            .unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "alpha");
        assert_eq!(hits[0].id, result.id);
    }

    #[test]
    fn get_by_id_prefix_works() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("tags", "alpha", None, None).unwrap();

        // Use first 3 chars as prefix
        let prefix = &result.id[..3];
        let hits = store
            .get_entries_by_id("tags", &[IdRef::Id(prefix.to_string())])
            .unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].value, "alpha");
    }

    #[test]
    fn get_by_mixed_id_types() {
        let (mut store, _dir) = setup_store(test_schema());
        let r1 = store.push("tags", "alpha", None, None).unwrap();
        let r2 = store.push("tags", "beta", None, None).unwrap();

        let hits = store
            .get_entries_by_id("tags", &[IdRef::Index(r1.index), IdRef::Id(r2.id.clone())])
            .unwrap();
        assert_eq!(hits.len(), 2);
    }

    #[test]
    fn remove_by_id_works() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        let r2 = store.push("tags", "beta", None, None).unwrap();
        store.push("tags", "gamma", None, None).unwrap();

        let result = store
            .remove("tags", None, Some(&IdRef::Id(r2.id.clone())), false)
            .unwrap();
        assert_eq!(result.removed.len(), 1);
        assert_eq!(result.removed[0], "beta");

        // Verify beta is gone
        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items.len(), 2);
                assert_eq!(items[0].value, "alpha");
                assert_eq!(items[1].value, "gamma");
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn remove_by_ambiguous_id_prefix_errors() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();

        // Manually set both entries to share an ID prefix.
        match store.data.entries.get_mut("tags").unwrap() {
            DataValue::List { items, .. } => {
                items[0].id = "ABCxyz1".to_string();
                items[1].id = "ABCxyz2".to_string();
            }
            _ => panic!("Expected list"),
        }

        // Removing by the shared prefix "ABC" should return an ambiguity error.
        let result = store.remove("tags", None, Some(&IdRef::Id("ABC".to_string())), false);
        assert!(result.is_err(), "expected ambiguity error");
        let err_msg = result.unwrap_err().to_string();
        assert!(
            err_msg.contains("ambiguous"),
            "error should mention ambiguity, got: {}",
            err_msg
        );
        assert!(
            err_msg.contains("matches 2 entries"),
            "error should report match count, got: {}",
            err_msg
        );
    }

    #[test]
    fn id_appears_in_last_output() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("tags", "alpha", None, None).unwrap();

        let items = store.last("tags", 1, None, &[]).unwrap();
        assert_eq!(items.len(), 1);
        assert_eq!(items[0].id, result.id);
    }

    #[test]
    fn id_appears_in_search_output() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("tags", "alpha", None, None).unwrap();

        let hits = store.search("tags", Some("alpha"), None, &[]).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].id, result.id);
    }

    #[test]
    fn id_stored_on_entry_structs() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("tags", "alpha", None, None).unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items[0].id, result.id);
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn id_stored_on_history_entry() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();

        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                assert_eq!(entries[0].id, result.id);
            }
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn id_persists_through_save_load() {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        let saved_id;
        {
            let mut store = KvStore::load(&schema_path, &data_path).unwrap();
            let result = store.push("tags", "alpha", None, None).unwrap();
            saved_id = result.id;
            store.save().unwrap();
        }

        // Reload and verify ID persisted
        let store2 = KvStore::load(&schema_path, &data_path).unwrap();
        match store2.data.entries.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items[0].id, saved_id);
            }
            _ => panic!("Expected list"),
        }
    }

    // -- Per-entry memory tests --

    #[test]
    fn push_with_memory_stores_on_entry() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("tags", "linked-item", None, Some("kn-abc123".to_string()))
            .unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items[0].memory, Some("kn-abc123".to_string()));
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn push_without_memory_has_none() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "plain-item", None, None).unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert!(items[0].memory.is_none());
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn push_history_with_memory() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push(
                "flavor_history",
                "bergamot",
                None,
                Some("kn-hist123".to_string()),
            )
            .unwrap();

        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                assert_eq!(entries[0].memory, Some("kn-hist123".to_string()));
            }
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn set_entry_memory_by_numeric_index() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("tags", "alpha", None, None).unwrap();

        store
            .set_entry_memory(
                "tags",
                &IdRef::Index(result.index),
                Some("kn-set1".to_string()),
            )
            .unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items[0].memory, Some("kn-set1".to_string()));
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn set_entry_memory_by_id() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store.push("tags", "alpha", None, None).unwrap();

        store
            .set_entry_memory(
                "tags",
                &IdRef::Id(result.id.clone()),
                Some("kn-id1".to_string()),
            )
            .unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items[0].memory, Some("kn-id1".to_string()));
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn set_entry_memory_not_found_errors() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();

        let result = store.set_entry_memory("tags", &IdRef::Index(999), Some("kn-x".to_string()));
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Entry not found"));
    }

    #[test]
    fn set_entry_memory_history_by_numeric_index() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store
            .push("flavor_history", "bergamot", None, None)
            .unwrap();

        store
            .set_entry_memory(
                "flavor_history",
                &IdRef::Index(result.index),
                Some("kn-hist-set".to_string()),
            )
            .unwrap();

        match store.get("flavor_history").unwrap() {
            DataValue::History { entries, .. } => {
                assert_eq!(entries[0].memory, Some("kn-hist-set".to_string()));
            }
            _ => panic!("Expected history"),
        }
    }

    #[test]
    fn per_entry_memory_in_search_hit() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("tags", "linked", None, Some("kn-search1".to_string()))
            .unwrap();
        store.push("tags", "plain", None, None).unwrap();

        let hits = store.search("tags", Some("linked"), None, &[]).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].memory, Some("kn-search1".to_string()));

        let hits = store.search("tags", Some("plain"), None, &[]).unwrap();
        assert_eq!(hits.len(), 1);
        assert!(hits[0].memory.is_none());
    }

    #[test]
    fn last_returns_search_hit_with_memory() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("tags", "with-mem", None, Some("kn-last1".to_string()))
            .unwrap();
        store.push("tags", "without-mem", None, None).unwrap();

        let hits = store.last("tags", 2, None, &[]).unwrap();
        assert_eq!(hits.len(), 2);
        assert_eq!(hits[0].memory, Some("kn-last1".to_string()));
        assert!(hits[1].memory.is_none());
    }

    #[test]
    fn random_returns_search_hit_with_memory() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("tags", "only-item", None, Some("kn-rand1".to_string()))
            .unwrap();

        let hits = store.random("tags", 1, None, &[]).unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].memory, Some("kn-rand1".to_string()));
    }

    #[test]
    fn backward_compat_data_without_memory_field() {
        // Simulate old data without memory field on entries
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        // Write data file without memory field on entries
        let old_data = r#"{
            "_schema": "test",
            "_updated": "2026-01-01T00:00:00Z",
            "tags": {
                "items": [
                    {"id": 1, "hash": "abc", "value": "old-item", "ts": "2026-01-01T00:00:00Z"}
                ]
            }
        }"#;
        fs::write(&data_path, old_data).unwrap();

        let store = KvStore::load(&schema_path, &data_path).unwrap();
        match store.data.entries.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items[0].value, "old-item");
                assert!(items[0].memory.is_none());
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn clear_entry_memory_with_empty_string() {
        let (mut store, _dir) = setup_store(test_schema());
        let result = store
            .push("tags", "alpha", None, Some("kn-clear1".to_string()))
            .unwrap();

        // Set memory to empty string should clear it
        store
            .set_entry_memory("tags", &IdRef::Index(result.index), Some("".to_string()))
            .unwrap();

        match store.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert!(items[0].memory.is_none());
            }
            _ => panic!("Expected list"),
        }
    }

    #[test]
    fn get_entries_by_id_carries_memory() {
        let (mut store, _dir) = setup_store(test_schema());
        let r1 = store
            .push("tags", "with-mem", None, Some("kn-byid1".to_string()))
            .unwrap();
        store.push("tags", "without", None, None).unwrap();

        let hits = store
            .get_entries_by_id("tags", &[IdRef::Index(r1.index)])
            .unwrap();
        assert_eq!(hits.len(), 1);
        assert_eq!(hits[0].memory, Some("kn-byid1".to_string()));
    }

    #[test]
    fn set_entry_memory_type_mismatch() {
        let (mut store, _dir) = setup_store(test_schema());
        store.inc("warmth", 1).unwrap();

        let result = store.set_entry_memory("warmth", &IdRef::Index(1), Some("kn-x".to_string()));
        assert!(result.is_err());
        assert!(result.unwrap_err().to_string().contains("Type mismatch"));
    }

    #[test]
    fn set_entry_memory_ambiguous_id_prefix_errors() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();

        // Manually set both entries to share an ID prefix.
        match store.data.entries.get_mut("tags").unwrap() {
            DataValue::List { items, .. } => {
                items[0].id = "XYZaaa1".to_string();
                items[1].id = "XYZaaa2".to_string();
            }
            _ => panic!("Expected list"),
        }

        // Setting memory by the shared prefix "XYZ" should return an ambiguity error.
        let result = store.set_entry_memory(
            "tags",
            &IdRef::Id("XYZ".to_string()),
            Some("kn-x".to_string()),
        );
        assert!(result.is_err(), "expected ambiguity error");
        let err_msg = result.unwrap_err().to_string();
        assert!(
            err_msg.contains("ambiguous"),
            "error should mention ambiguity, got: {}",
            err_msg
        );
        assert!(
            err_msg.contains("matches 2 entries"),
            "error should report match count, got: {}",
            err_msg
        );
    }

    #[test]
    fn entry_memory_persists_through_save_reload() {
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        let mut f = fs::File::create(&schema_path).unwrap();
        f.write_all(test_schema().as_bytes()).unwrap();

        {
            let mut store = KvStore::load(&schema_path, &data_path).unwrap();
            store
                .push("tags", "persistent", None, Some("kn-persist".to_string()))
                .unwrap();
            store.save().unwrap();
        }

        let store2 = KvStore::load(&schema_path, &data_path).unwrap();
        match store2.data.entries.get("tags").unwrap() {
            DataValue::List { items, .. } => {
                assert_eq!(items[0].memory, Some("kn-persist".to_string()));
            }
            _ => panic!("Expected list"),
        }
    }

    // -- add_key_to_schema --

    #[test]
    fn add_key_to_schema_history() {
        let (mut store, _dir) = setup_store(test_schema());
        assert!(!store.schema.keys.contains_key("puns"));

        store.add_key_to_schema("puns", "history", None).unwrap();

        // Key should now exist in the in-memory schema
        let def = store.schema.keys.get("puns").unwrap();
        assert_eq!(def.value_type, ValueType::History);
        assert_eq!(def.max_entries, None);

        // Push should now work on the new key
        store.push("puns", "the joke", None, None).unwrap();
        let last = store.last("puns", 1, None, &[]).unwrap();
        assert_eq!(last.len(), 1);
        assert_eq!(last[0].value, "the joke");
    }

    #[test]
    fn add_key_to_schema_list() {
        let (mut store, _dir) = setup_store(test_schema());
        assert!(!store.schema.keys.contains_key("items"));

        store.add_key_to_schema("items", "list", None).unwrap();

        let def = store.schema.keys.get("items").unwrap();
        assert_eq!(def.value_type, ValueType::List);

        store.push("items", "apple", None, None).unwrap();
        let last = store.last("items", 1, None, &[]).unwrap();
        assert_eq!(last[0].value, "apple");
    }

    #[test]
    fn add_key_to_schema_with_max_entries() {
        let (mut store, _dir) = setup_store(test_schema());

        store
            .add_key_to_schema("puns", "history", Some(500))
            .unwrap();

        let def = store.schema.keys.get("puns").unwrap();
        assert_eq!(def.value_type, ValueType::History);
        assert_eq!(def.max_entries, Some(500));
    }

    #[test]
    fn add_key_to_schema_existing_key_is_noop() {
        let (mut store, _dir) = setup_store(test_schema());

        // "tags" already exists as a list in the test schema
        assert!(store.schema.keys.contains_key("tags"));
        let original_type = store.schema.keys["tags"].value_type;

        // Should succeed silently without changing anything
        store.add_key_to_schema("tags", "history", None).unwrap();

        // Type should be unchanged
        assert_eq!(store.schema.keys["tags"].value_type, original_type);
    }

    // Type validation for --create is now enforced by clap's ValueEnum
    // (CreateType), so there is no add_key_to_schema_invalid_type_rejected
    // test -- invalid types cannot reach add_key_to_schema.

    #[test]
    fn add_key_to_schema_dotted_name_rejected() {
        let (mut store, _dir) = setup_store(test_schema());

        let err = store
            .add_key_to_schema("my.key", "history", None)
            .unwrap_err();
        assert!(err.to_string().contains("cannot contain dots"));
    }

    #[test]
    fn add_key_to_schema_special_chars_rejected() {
        let (mut store, _dir) = setup_store(test_schema());

        let err = store
            .add_key_to_schema("my key!", "history", None)
            .unwrap_err();
        assert!(err.to_string().contains("invalid characters"));
    }

    #[test]
    fn add_key_to_schema_empty_name_rejected() {
        let (mut store, _dir) = setup_store(test_schema());

        let err = store.add_key_to_schema("", "history", None).unwrap_err();
        assert!(err.to_string().contains("cannot be empty"));
    }

    #[test]
    fn add_key_to_schema_reparsed_correctly() {
        let (mut store, _dir) = setup_store(test_schema());
        let original_key_count = store.schema.keys.len();

        store
            .add_key_to_schema("new_key", "history", Some(50))
            .unwrap();

        // Should have exactly one more key
        assert_eq!(store.schema.keys.len(), original_key_count + 1);

        // Reload from disk to verify file was written correctly
        let store2 = KvStore::load(&store.schema_path, &store.data_path).unwrap();
        assert_eq!(store2.schema.keys.len(), original_key_count + 1);
        let def = store2.schema.keys.get("new_key").unwrap();
        assert_eq!(def.value_type, ValueType::History);
        assert_eq!(def.max_entries, Some(50));
    }

    #[test]
    fn add_key_to_schema_preserves_existing_content() {
        let (mut store, _dir) = setup_store(test_schema());

        // Push some data to an existing key first
        store.push("tags", "hello", None, None).unwrap();
        store.save().unwrap();

        // Now add a new key to schema
        store.add_key_to_schema("jokes", "list", None).unwrap();

        // Original keys should all still be present and correct
        assert_eq!(store.schema.keys["warmth"].value_type, ValueType::Counter);
        assert_eq!(
            store.schema.keys["flavor_history"].value_type,
            ValueType::History
        );
        assert_eq!(store.schema.keys["tags"].value_type, ValueType::List);
        assert_eq!(store.schema.keys["tensor"].value_type, ValueType::State);
        assert_eq!(
            store.schema.keys["current_mood"].value_type,
            ValueType::String
        );

        // New key should be there too
        assert_eq!(store.schema.keys["jokes"].value_type, ValueType::List);
    }

    #[test]
    fn add_key_hyphen_and_underscore_accepted() {
        let (mut store, _dir) = setup_store(test_schema());

        store
            .add_key_to_schema("my-key_v2", "history", None)
            .unwrap();
        assert!(store.schema.keys.contains_key("my-key_v2"));
    }

    #[test]
    fn add_key_to_schema_name_too_long_rejected() {
        let (mut store, _dir) = setup_store(test_schema());
        let long_name = "a".repeat(129);

        let err = store
            .add_key_to_schema(&long_name, "history", None)
            .unwrap_err();
        assert!(err.to_string().contains("key name too long"));
        assert!(err.to_string().contains("129 chars"));
        assert!(err.to_string().contains("max 128"));
    }

    #[test]
    fn add_key_to_schema_max_length_accepted() {
        let (mut store, _dir) = setup_store(test_schema());
        let max_name = "a".repeat(128);

        store.add_key_to_schema(&max_name, "history", None).unwrap();
        assert!(store.schema.keys.contains_key(max_name.as_str()));
    }

    #[test]
    fn add_key_to_schema_without_trailing_newline() {
        // S2: Schema file without trailing newline should still produce valid TOML
        // after add_key_to_schema appends a new key block.
        let schema_no_newline = "[keys.existing]\ntype = \"history\"";
        let dir = TempDir::new().unwrap();
        let schema_path = dir.path().join("test.schema.toml");
        let data_path = dir.path().join("test.data.json");

        // Write schema WITHOUT trailing newline
        fs::write(&schema_path, schema_no_newline).unwrap();

        let mut store = KvStore::load(&schema_path, &data_path).unwrap();
        assert!(store.schema.keys.contains_key("existing"));

        // Add a new key -- should not corrupt the file
        store.add_key_to_schema("newkey", "list", None).unwrap();

        // Both keys should be present after re-parse
        assert!(store.schema.keys.contains_key("existing"));
        assert!(store.schema.keys.contains_key("newkey"));
        assert_eq!(store.schema.keys["existing"].value_type, ValueType::History);
        assert_eq!(store.schema.keys["newkey"].value_type, ValueType::List);

        // Verify file on disk is valid TOML by reloading
        let store2 = KvStore::load(&schema_path, &data_path).unwrap();
        assert!(store2.schema.keys.contains_key("existing"));
        assert!(store2.schema.keys.contains_key("newkey"));
    }

    // -- SearchHit JSON serialization --

    #[test]
    fn search_hit_serializes_all_fields() {
        let hit = SearchHit {
            index: 7,
            id: "A3fB".to_string(),
            value: "test entry".to_string(),
            ts: "2026-05-08T12:00:00Z".to_string(),
            data: Some(serde_json::json!({"status": "active"})),
            memory: Some("kn-abc123".to_string()),
        };
        let json_str = serde_json::to_string_pretty(&hit).unwrap();
        let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap();

        assert_eq!(parsed["index"], 7);
        assert_eq!(parsed["id"], "A3fB");
        assert_eq!(parsed["value"], "test entry");
        assert_eq!(parsed["ts"], "2026-05-08T12:00:00Z");
        assert_eq!(parsed["data"]["status"], "active");
        assert_eq!(parsed["memory"], "kn-abc123");
    }

    #[test]
    fn search_hit_omits_null_data_and_memory() {
        let hit = SearchHit {
            index: 1,
            id: "Bf2C".to_string(),
            value: "no extras".to_string(),
            ts: "2026-05-08T12:00:00Z".to_string(),
            data: None,
            memory: None,
        };
        let json_str = serde_json::to_string(&hit).unwrap();
        let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap();

        assert!(
            parsed.get("data").is_none(),
            "data should be omitted when None"
        );
        assert!(
            parsed.get("memory").is_none(),
            "memory should be omitted when None"
        );
        // Required fields are present
        assert!(parsed.get("index").is_some());
        assert!(parsed.get("id").is_some());
        assert!(parsed.get("value").is_some());
        assert!(parsed.get("ts").is_some());
    }

    #[test]
    fn search_hit_vec_serializes_as_json_array() {
        let hits = vec![
            SearchHit {
                index: 1,
                id: "aaa".to_string(),
                value: "first".to_string(),
                ts: "2026-05-08T10:00:00Z".to_string(),
                data: None,
                memory: None,
            },
            SearchHit {
                index: 2,
                id: "bbb".to_string(),
                value: "second".to_string(),
                ts: "2026-05-08T11:00:00Z".to_string(),
                data: Some(serde_json::json!({"priority": "high"})),
                memory: Some("kn-xyz".to_string()),
            },
        ];
        let json_str = serde_json::to_string_pretty(&hits).unwrap();
        let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap();

        assert!(parsed.is_array());
        let arr = parsed.as_array().unwrap();
        assert_eq!(arr.len(), 2);
        assert_eq!(arr[0]["value"], "first");
        assert!(arr[0].get("data").is_none());
        assert_eq!(arr[1]["value"], "second");
        assert_eq!(arr[1]["data"]["priority"], "high");
        assert_eq!(arr[1]["memory"], "kn-xyz");
    }

    #[test]
    fn search_hit_uses_rust_field_names_not_disk_renames() {
        // SearchHit deliberately uses Rust field names (index, id) for JSON output,
        // NOT the on-disk serde rename aliases used by HistoryEntry/ListEntry.
        let hit = SearchHit {
            index: 42,
            id: "test".to_string(),
            value: "v".to_string(),
            ts: "2026-01-01T00:00:00Z".to_string(),
            data: None,
            memory: None,
        };
        let json_str = serde_json::to_string(&hit).unwrap();
        assert!(json_str.contains("\"index\""), "should use 'index' not 'i'");
        assert!(json_str.contains("\"id\""), "should use 'id' not 'h'");
        assert!(
            !json_str.contains("\"i\":"),
            "should NOT use on-disk 'i' alias"
        );
        assert!(
            !json_str.contains("\"h\":"),
            "should NOT use on-disk 'h' alias"
        );
    }

    #[test]
    fn since_json_uses_search_hit_field_names() {
        // Verify that converting HistoryEntry to SearchHit for since --json
        // produces "index" and "id" field names, not the on-disk renames
        // ("id" for index, "hash" for id) that HistoryEntry uses.
        let (mut store, _dir) = setup_store(test_schema());

        let ts = Utc::now() - chrono::Duration::minutes(5);
        store
            .push_with_ts("flavor_history", "recent_flavor", ts, None, None)
            .unwrap();

        let entries = store.since("flavor_history", "1h").unwrap();
        assert_eq!(entries.len(), 1);

        // Convert to SearchHit (same transform the handler does)
        let hits: Vec<SearchHit> = entries
            .iter()
            .map(|e| SearchHit {
                index: e.index,
                id: e.id.clone(),
                value: e.value.clone(),
                ts: e.ts.clone(),
                data: e.data.clone(),
                memory: e.memory.clone(),
            })
            .collect();

        let json_str = serde_json::to_string_pretty(&hits).unwrap();
        let parsed: Vec<serde_json::Value> = serde_json::from_str(&json_str).unwrap();

        assert_eq!(parsed.len(), 1);
        let obj = &parsed[0];

        // Must have "index" (numeric) and "id" (string hash)
        assert!(
            obj.get("index").is_some(),
            "should have 'index' field, got: {}",
            json_str
        );
        assert!(
            obj.get("id").is_some(),
            "should have 'id' field, got: {}",
            json_str
        );
        assert!(obj["index"].is_number(), "index should be a number");
        assert!(obj["id"].is_string(), "id should be a string (hash)");

        // Must NOT have the on-disk rename aliases
        assert!(
            obj.get("hash").is_none(),
            "should NOT have 'hash' field (on-disk alias for id)"
        );

        // "id" should be the hash, not the numeric index
        assert_eq!(obj["value"], "recent_flavor");
    }

    #[test]
    fn count_json_includes_total_and_latest_ts() {
        // Verify the count result carries total and latest_ts for filtered queries
        // so that --json output can include them.
        let (mut store, _dir) = setup_store(test_schema());

        let ts1 = DateTime::parse_from_rfc3339("2026-04-20T10:00:00Z")
            .unwrap()
            .with_timezone(&Utc);
        let ts2 = DateTime::parse_from_rfc3339("2026-04-22T15:00:00Z")
            .unwrap()
            .with_timezone(&Utc);

        store
            .push_with_ts("flavor_history", "bergamot", ts1, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "lapsang", ts1, None, None)
            .unwrap();
        store
            .push_with_ts("flavor_history", "bergamot earl grey", ts2, None, None)
            .unwrap();

        let result = store
            .count("flavor_history", Some("bergamot"), None, &[])
            .unwrap();

        // Build JSON the same way the handler does
        let mut json_val = serde_json::json!({"count": result.matched});
        if let Some(total) = result.total {
            json_val["total"] = serde_json::json!(total);
        }
        if let Some(ref ts) = result.latest_ts {
            json_val["latest_ts"] = serde_json::json!(ts);
        }

        let obj: serde_json::Value = json_val;

        assert_eq!(obj["count"], 2, "2 entries match 'bergamot'");
        assert_eq!(obj["total"], 3, "3 total entries");
        assert!(
            obj.get("latest_ts").is_some(),
            "should include latest_ts for filtered count"
        );
        let latest = obj["latest_ts"].as_str().unwrap();
        assert!(
            latest.contains("2026-04-22"),
            "latest_ts should be the most recent matching entry"
        );
    }

    // -- Data field schema parsing --

    fn data_schema() -> &'static str {
        r#"
[keys.projects]
type = "history"
max_entries = 100

[keys.projects.data]
status = { type = "string", required = true }
tags = { type = "array" }
repo = { type = "string" }
priority = { type = "number" }

[keys.notes]
type = "history"
"#
    }

    #[test]
    fn parse_schema_with_data_fields() {
        let (store, _dir) = setup_store(data_schema());
        let projects = &store.schema.keys["projects"];
        let data_defs = projects.data.as_ref().expect("data should be Some");
        assert_eq!(data_defs.len(), 4);
        assert_eq!(data_defs["status"].field_type, DataFieldType::String);
        assert!(data_defs["status"].required);
        assert_eq!(data_defs["tags"].field_type, DataFieldType::Array);
        assert!(!data_defs["tags"].required);
        assert_eq!(data_defs["repo"].field_type, DataFieldType::String);
        assert_eq!(data_defs["priority"].field_type, DataFieldType::Number);
    }

    #[test]
    fn parse_schema_data_field_types() {
        let schema = r#"
[keys.all_types]
type = "history"

[keys.all_types.data]
s = { type = "string" }
n = { type = "number" }
b = { type = "boolean" }
a = { type = "array" }
o = { type = "object" }
"#;
        let (store, _dir) = setup_store(schema);
        let defs = store.schema.keys["all_types"]
            .data
            .as_ref()
            .expect("data should be Some");
        assert_eq!(defs["s"].field_type, DataFieldType::String);
        assert_eq!(defs["n"].field_type, DataFieldType::Number);
        assert_eq!(defs["b"].field_type, DataFieldType::Boolean);
        assert_eq!(defs["a"].field_type, DataFieldType::Array);
        assert_eq!(defs["o"].field_type, DataFieldType::Object);
    }

    #[test]
    fn parse_schema_data_field_required_and_default() {
        let schema = r#"
[keys.test]
type = "history"

[keys.test.data]
name = { type = "string", required = true, default = "unnamed" }
opt = { type = "string" }
"#;
        let (store, _dir) = setup_store(schema);
        let defs = store.schema.keys["test"]
            .data
            .as_ref()
            .expect("data should be Some");
        assert!(defs["name"].required);
        assert_eq!(defs["name"].default.as_deref(), Some("unnamed"));
        assert!(!defs["opt"].required);
        assert!(defs["opt"].default.is_none());
    }

    #[test]
    fn parse_schema_without_data_preserves_none() {
        let (store, _dir) = setup_store(data_schema());
        assert!(
            store.schema.keys["notes"].data.is_none(),
            "key without [data] section should have data: None"
        );
    }

    // -- validate_data unit tests --

    fn sample_key_def() -> KeyDef {
        let mut data = BTreeMap::new();
        data.insert(
            "status".to_string(),
            DataFieldDef {
                field_type: DataFieldType::String,
                required: true,
                default: None,
            },
        );
        data.insert(
            "tags".to_string(),
            DataFieldDef {
                field_type: DataFieldType::Array,
                required: false,
                default: None,
            },
        );
        data.insert(
            "priority".to_string(),
            DataFieldDef {
                field_type: DataFieldType::Number,
                required: false,
                default: None,
            },
        );
        KeyDef {
            value_type: ValueType::History,
            min: None,
            max: None,
            default: None,
            max_entries: None,
            description: None,
            fields: None,
            data: Some(data),
        }
    }

    fn key_def_with_data(data: BTreeMap<String, DataFieldDef>) -> KeyDef {
        KeyDef {
            value_type: ValueType::History,
            min: None,
            max: None,
            default: None,
            max_entries: None,
            description: None,
            fields: None,
            data: Some(data),
        }
    }

    #[test]
    fn validate_data_accepts_valid() {
        let def = sample_key_def();
        let data = Some(serde_json::json!({
            "status": "active",
            "tags": ["a", "b"],
            "priority": 5
        }));
        assert!(def.validate_data("test", &data).is_ok());
    }

    #[test]
    fn validate_data_rejects_extra_field() {
        let def = sample_key_def();
        let data = Some(serde_json::json!({
            "status": "active",
            "unknown_field": true
        }));
        let err = def.validate_data("test", &data).unwrap_err();
        let msg = err.to_string();
        assert!(
            msg.contains("undeclared"),
            "error should mention undeclared: {}",
            msg
        );
        assert!(
            msg.contains("unknown_field"),
            "error should name the extra field: {}",
            msg
        );
        assert!(
            msg.contains("priority"),
            "error should list declared fields: {}",
            msg
        );
    }

    #[test]
    fn validate_data_rejects_missing_required() {
        let def = sample_key_def();
        let data = Some(serde_json::json!({
            "tags": ["a"]
        }));
        let err = def.validate_data("test", &data).unwrap_err();
        let msg = err.to_string();
        assert!(
            msg.contains("missing required"),
            "error should say missing required: {}",
            msg
        );
        assert!(
            msg.contains("status"),
            "error should name the missing field: {}",
            msg
        );
    }

    #[test]
    fn validate_data_allows_missing_optional() {
        let def = sample_key_def();
        let data = Some(serde_json::json!({
            "status": "active"
        }));
        assert!(def.validate_data("test", &data).is_ok());
    }

    #[test]
    fn validate_data_rejects_type_mismatch() {
        let def = sample_key_def();
        let data = Some(serde_json::json!({
            "status": 42
        }));
        let err = def.validate_data("test", &data).unwrap_err();
        let msg = err.to_string();
        assert!(
            msg.contains("must be string"),
            "error should say expected type: {}",
            msg
        );
        assert!(
            msg.contains("got number"),
            "error should say actual type: {}",
            msg
        );
    }

    #[test]
    fn validate_data_batches_type_mismatches() {
        let def = sample_key_def();
        let data = Some(serde_json::json!({
            "status": 42,
            "tags": "not-an-array",
            "priority": true
        }));
        let err = def.validate_data("test", &data).unwrap_err();
        let msg = err.to_string();
        assert!(msg.contains("'status'"), "should name status: {}", msg);
        assert!(msg.contains("'tags'"), "should name tags: {}", msg);
        assert!(msg.contains("'priority'"), "should name priority: {}", msg);
    }

    #[test]
    fn validate_data_all_types() {
        let mut defs = BTreeMap::new();
        defs.insert(
            "s".to_string(),
            DataFieldDef {
                field_type: DataFieldType::String,
                required: false,
                default: None,
            },
        );
        defs.insert(
            "n".to_string(),
            DataFieldDef {
                field_type: DataFieldType::Number,
                required: false,
                default: None,
            },
        );
        defs.insert(
            "b".to_string(),
            DataFieldDef {
                field_type: DataFieldType::Boolean,
                required: false,
                default: None,
            },
        );
        defs.insert(
            "a".to_string(),
            DataFieldDef {
                field_type: DataFieldType::Array,
                required: false,
                default: None,
            },
        );
        defs.insert(
            "o".to_string(),
            DataFieldDef {
                field_type: DataFieldType::Object,
                required: false,
                default: None,
            },
        );
        let def = key_def_with_data(defs);

        let data = Some(serde_json::json!({
            "s": "hello",
            "n": 42.5,
            "b": true,
            "a": [1, 2, 3],
            "o": {"nested": true}
        }));
        assert!(def.validate_data("test", &data).is_ok());

        let bad = Some(serde_json::json!({ "s": 42 }));
        assert!(def.validate_data("test", &bad).is_err());

        let bad = Some(serde_json::json!({ "n": "nope" }));
        assert!(def.validate_data("test", &bad).is_err());

        let bad = Some(serde_json::json!({ "b": "true" }));
        assert!(def.validate_data("test", &bad).is_err());

        let bad = Some(serde_json::json!({ "a": {"not": "array"} }));
        assert!(def.validate_data("test", &bad).is_err());

        let bad = Some(serde_json::json!({ "o": [1, 2] }));
        assert!(def.validate_data("test", &bad).is_err());
    }

    #[test]
    fn validate_data_empty_object_all_optional() {
        let mut defs = BTreeMap::new();
        defs.insert(
            "opt1".to_string(),
            DataFieldDef {
                field_type: DataFieldType::String,
                required: false,
                default: None,
            },
        );
        defs.insert(
            "opt2".to_string(),
            DataFieldDef {
                field_type: DataFieldType::Number,
                required: false,
                default: None,
            },
        );
        let def = key_def_with_data(defs);

        let data = Some(serde_json::json!({}));
        assert!(def.validate_data("test", &data).is_ok());
    }

    #[test]
    fn validate_data_empty_object_with_required() {
        let mut defs = BTreeMap::new();
        defs.insert(
            "req".to_string(),
            DataFieldDef {
                field_type: DataFieldType::String,
                required: true,
                default: None,
            },
        );
        defs.insert(
            "opt".to_string(),
            DataFieldDef {
                field_type: DataFieldType::Number,
                required: false,
                default: None,
            },
        );
        let def = key_def_with_data(defs);

        let data = Some(serde_json::json!({}));
        let err = def.validate_data("test", &data).unwrap_err();
        assert!(err.to_string().contains("missing required"));
        assert!(err.to_string().contains("req"));
    }

    // -- Integration: push with data validation --

    #[test]
    fn push_with_valid_data_and_schema() {
        let (mut store, _dir) = setup_store(data_schema());
        let data = serde_json::json!({
            "status": "active",
            "tags": ["rust"],
            "repo": "mx",
            "priority": 1
        });
        let result = store.push("projects", "my-project", Some(data.clone()), None);
        assert!(result.is_ok(), "push with valid data should succeed");

        // Verify the entry is stored
        match &store.data.entries["projects"] {
            DataValue::History { entries, .. } => {
                assert_eq!(entries.len(), 1);
                assert_eq!(entries[0].data.as_ref().unwrap(), &data);
            }
            _ => panic!("expected History"),
        }
    }

    #[test]
    fn push_with_invalid_data_rejected() {
        let (mut store, _dir) = setup_store(data_schema());
        let data = serde_json::json!({
            "status": 42
        });
        let result = store.push("projects", "bad-project", Some(data), None);
        assert!(result.is_err(), "push with invalid data should fail");
        let err = result.unwrap_err();
        assert!(
            matches!(err, KvError::DataValidation { .. }),
            "error should be DataValidation, got: {:?}",
            err
        );

        // Verify no entry was stored
        assert!(
            !store.data.entries.contains_key("projects"),
            "rejected push should not create an entry"
        );
    }

    #[test]
    fn push_without_data_when_required() {
        let (mut store, _dir) = setup_store(data_schema());
        let result = store.push("projects", "no-data", None, None);
        assert!(
            result.is_err(),
            "push without data when schema has required fields should fail"
        );
        let err = result.unwrap_err();
        let msg = err.to_string();
        assert!(
            msg.contains("--data is required"),
            "error should mention --data: {}",
            msg
        );
        assert!(
            msg.contains("status"),
            "error should name the required field: {}",
            msg
        );
    }

    #[test]
    fn push_without_data_when_all_optional() {
        let schema = r#"
[keys.loose]
type = "history"

[keys.loose.data]
note = { type = "string" }
count = { type = "number" }
"#;
        let (mut store, _dir) = setup_store(schema);
        let result = store.push("loose", "no-data-ok", None, None);
        assert!(
            result.is_ok(),
            "push without data when all fields optional should succeed"
        );

        match &store.data.entries["loose"] {
            DataValue::History { entries, .. } => {
                assert_eq!(entries.len(), 1);
                assert!(entries[0].data.is_none(), "data should be None");
            }
            _ => panic!("expected History"),
        }
    }

    #[test]
    fn push_without_data_freeform() {
        let (mut store, _dir) = setup_store(data_schema());
        // "notes" has no [keys.notes.data] section -- freeform
        let result = store.push("notes", "anything goes", None, None);
        assert!(
            result.is_ok(),
            "push without data on freeform key should succeed"
        );

        match &store.data.entries["notes"] {
            DataValue::History { entries, .. } => {
                assert_eq!(entries.len(), 1);
                assert!(entries[0].data.is_none());
            }
            _ => panic!("expected History"),
        }
    }

    // -- Null handling --

    #[test]
    fn validate_data_null_optional_field_treated_as_absent() {
        let def = sample_key_def();
        let data = Some(serde_json::json!({
            "status": "active",
            "tags": null
        }));
        assert!(
            def.validate_data("test", &data).is_ok(),
            "null on optional field should be treated as absent"
        );
    }

    #[test]
    fn validate_data_null_required_field_is_missing() {
        let def = sample_key_def();
        let data = Some(serde_json::json!({
            "status": null
        }));
        let err = def.validate_data("test", &data).unwrap_err();
        let msg = err.to_string();
        assert!(
            msg.contains("missing required"),
            "null on required field = missing: {}",
            msg
        );
        assert!(msg.contains("status"), "{}", msg);
    }

    #[test]
    fn validate_data_null_undeclared_field_ignored() {
        let def = sample_key_def();
        let data = Some(serde_json::json!({
            "status": "active",
            "ghost": null
        }));
        assert!(
            def.validate_data("test", &data).is_ok(),
            "null undeclared field should be ignored (treated as absent)"
        );
    }

    #[test]
    fn validate_data_rejects_non_object() {
        let def = sample_key_def();
        let array = Some(serde_json::json!(["not", "an", "object"]));
        let err = def.validate_data("test", &array).unwrap_err();
        assert!(err.to_string().contains("must be a JSON object"));

        let string = Some(serde_json::json!("just a string"));
        let err = def.validate_data("test", &string).unwrap_err();
        assert!(err.to_string().contains("must be a JSON object"));
    }

    #[test]
    fn validate_data_none_passes_freeform() {
        let def = KeyDef {
            value_type: ValueType::History,
            min: None,
            max: None,
            default: None,
            max_entries: None,
            description: None,
            fields: None,
            data: None,
        };
        assert!(def.validate_data("test", &None).is_ok());
        let data = Some(serde_json::json!({"anything": "goes"}));
        assert!(def.validate_data("test", &data).is_ok());
    }

    // -- migrate tests --

    fn migrate_schema() -> &'static str {
        r#"
[keys.projects]
type = "history"
max_entries = 100

[keys.projects.data]
status = { type = "string", required = true, default = "unknown" }
tags = { type = "array" }
priority = { type = "number", default = "0" }

[keys.tasks]
type = "list"
max_entries = 50

[keys.tasks.data]
title = { type = "string", required = true }
done = { type = "boolean", default = "false" }

[keys.notes]
type = "history"

[keys.visits]
type = "counter"
"#
    }

    #[test]
    fn migrate_fills_defaults() {
        let (mut store, _dir) = setup_store(migrate_schema());
        // Push an entry missing the required "status" field (bypass validation
        // by inserting directly).
        store.data.entries.insert(
            "projects".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "abc1".to_string(),
                    value: "proj-a".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: Some(serde_json::json!({"tags": ["rust"]})),
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("projects", false, false).unwrap();
        assert_eq!(result.examined, 1);
        assert_eq!(result.modified, 1);
        assert_eq!(result.changes.len(), 1);
        assert!(
            result.changes[0]
                .fields_added
                .contains(&"status".to_string())
        );
        assert!(
            result.changes[0]
                .fields_added
                .contains(&"priority".to_string())
        );

        // Verify the entry was actually mutated
        if let DataValue::History { entries, .. } = &store.data.entries["projects"] {
            let data = entries[0].data.as_ref().unwrap();
            assert_eq!(data["status"], serde_json::json!("unknown"));
            assert_eq!(data["priority"], serde_json::json!(0));
        } else {
            panic!("expected history");
        }
    }

    #[test]
    fn migrate_skips_conforming_entries() {
        let (mut store, _dir) = setup_store(migrate_schema());
        store.data.entries.insert(
            "projects".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "abc1".to_string(),
                    value: "proj-a".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: Some(
                        serde_json::json!({"status": "active", "tags": ["rust"], "priority": 5}),
                    ),
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("projects", false, false).unwrap();
        assert_eq!(result.examined, 1);
        assert_eq!(result.modified, 0);
        assert!(result.changes.is_empty());
    }

    #[test]
    fn migrate_prune_removes_undeclared() {
        let (mut store, _dir) = setup_store(migrate_schema());
        store.data.entries.insert(
            "projects".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "abc1".to_string(),
                    value: "proj-a".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: Some(serde_json::json!({
                        "status": "active",
                        "tags": ["rust"],
                        "priority": 5,
                        "obsolete_field": "should be removed"
                    })),
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("projects", true, false).unwrap();
        assert_eq!(result.examined, 1);
        assert_eq!(result.modified, 1);
        assert!(
            result.changes[0]
                .fields_pruned
                .contains(&"obsolete_field".to_string())
        );

        // Verify it was removed
        if let DataValue::History { entries, .. } = &store.data.entries["projects"] {
            let data = entries[0].data.as_ref().unwrap();
            assert!(data.get("obsolete_field").is_none());
        } else {
            panic!("expected history");
        }
    }

    #[test]
    fn migrate_no_prune_leaves_extra_fields() {
        let (mut store, _dir) = setup_store(migrate_schema());
        store.data.entries.insert(
            "projects".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "abc1".to_string(),
                    value: "proj-a".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: Some(serde_json::json!({
                        "status": "active",
                        "tags": ["rust"],
                        "priority": 5,
                        "extra": "stays"
                    })),
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("projects", false, false).unwrap();
        assert_eq!(result.modified, 0);

        // Verify extra field is still there
        if let DataValue::History { entries, .. } = &store.data.entries["projects"] {
            let data = entries[0].data.as_ref().unwrap();
            assert_eq!(data["extra"], serde_json::json!("stays"));
        } else {
            panic!("expected history");
        }
    }

    #[test]
    fn migrate_warns_on_type_mismatch() {
        let (mut store, _dir) = setup_store(migrate_schema());
        store.data.entries.insert(
            "projects".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "abc1".to_string(),
                    value: "proj-a".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: Some(serde_json::json!({
                        "status": 42,
                        "priority": "not-a-number"
                    })),
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("projects", false, false).unwrap();
        // Should have warnings about type mismatches
        let type_warnings: Vec<&String> = result
            .warnings
            .iter()
            .filter(|w| w.contains("type") || w.contains("expects"))
            .collect();
        assert!(
            type_warnings.len() >= 2,
            "expected warnings for status and priority type mismatches, got: {:?}",
            result.warnings
        );

        // The mismatched fields should not be modified
        if let DataValue::History { entries, .. } = &store.data.entries["projects"] {
            let data = entries[0].data.as_ref().unwrap();
            assert_eq!(
                data["status"],
                serde_json::json!(42),
                "status should not be coerced"
            );
            assert_eq!(
                data["priority"],
                serde_json::json!("not-a-number"),
                "priority should not be coerced"
            );
        } else {
            panic!("expected history");
        }
    }

    #[test]
    fn migrate_warns_on_required_without_default() {
        let (mut store, _dir) = setup_store(migrate_schema());
        // tasks.title is required but has no default
        store.data.entries.insert(
            "tasks".to_string(),
            DataValue::List {
                items: vec![ListEntry {
                    index: 1,
                    id: "task1".to_string(),
                    value: "do stuff".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: Some(serde_json::json!({})),
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("tasks", false, false).unwrap();
        let has_warning = result
            .warnings
            .iter()
            .any(|w| w.contains("title") && w.contains("required") && w.contains("no default"));
        assert!(
            has_warning,
            "should warn about missing required field without default: {:?}",
            result.warnings
        );
    }

    #[test]
    fn migrate_creates_data_blob_from_defaults() {
        let (mut store, _dir) = setup_store(migrate_schema());
        // Entry with data: None
        store.data.entries.insert(
            "projects".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "abc1".to_string(),
                    value: "proj-a".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: None,
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("projects", false, false).unwrap();
        assert_eq!(result.modified, 1);
        assert!(
            result.changes[0]
                .fields_added
                .contains(&"status".to_string())
        );
        assert!(
            result.changes[0]
                .fields_added
                .contains(&"priority".to_string())
        );

        // Verify data blob was created
        if let DataValue::History { entries, .. } = &store.data.entries["projects"] {
            let data = entries[0].data.as_ref().expect("data should now be Some");
            assert_eq!(data["status"], serde_json::json!("unknown"));
            assert_eq!(data["priority"], serde_json::json!(0));
        } else {
            panic!("expected history");
        }
    }

    #[test]
    fn migrate_dry_run_no_mutation() {
        let (mut store, _dir) = setup_store(migrate_schema());
        store.data.entries.insert(
            "projects".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "abc1".to_string(),
                    value: "proj-a".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: Some(serde_json::json!({"tags": ["rust"]})),
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("projects", false, true).unwrap();
        assert_eq!(result.modified, 1, "dry run should report modifications");
        assert!(!result.changes[0].fields_added.is_empty());

        // Verify no actual mutation
        if let DataValue::History { entries, .. } = &store.data.entries["projects"] {
            let data = entries[0].data.as_ref().unwrap();
            assert!(
                data.get("status").is_none(),
                "dry run should not insert status"
            );
            assert!(
                data.get("priority").is_none(),
                "dry run should not insert priority"
            );
        } else {
            panic!("expected history");
        }
    }

    #[test]
    fn migrate_no_data_defs_reports_nothing() {
        let (mut store, _dir) = setup_store(migrate_schema());
        // "notes" has no [data] section
        store.data.entries.insert(
            "notes".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "n1".to_string(),
                    value: "a note".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: None,
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("notes", false, false).unwrap();
        assert_eq!(result.examined, 0);
        assert_eq!(result.modified, 0);
        assert!(
            result
                .warnings
                .iter()
                .any(|w| w.contains("nothing to migrate")),
            "should report nothing to migrate: {:?}",
            result.warnings
        );
    }

    #[test]
    fn migrate_rejects_non_entry_type() {
        let (mut store, _dir) = setup_store(migrate_schema());
        store
            .data
            .entries
            .insert("visits".to_string(), DataValue::Counter { value: 5 });

        let err = store.migrate("visits", false, false).unwrap_err();
        let msg = err.to_string();
        assert!(
            msg.contains("counter"),
            "error should mention the type: {}",
            msg
        );
        assert!(
            msg.contains("migrate only works on"),
            "error should explain constraint: {}",
            msg
        );
    }

    // -- parse_default_value tests --

    #[test]
    fn parse_default_value_string() {
        let val = parse_default_value("unknown", DataFieldType::String).unwrap();
        assert_eq!(val, serde_json::Value::String("unknown".to_string()));
    }

    #[test]
    fn parse_default_value_number() {
        let val = parse_default_value("0", DataFieldType::Number).unwrap();
        assert_eq!(val, serde_json::json!(0));
    }

    #[test]
    fn parse_default_value_boolean() {
        let val = parse_default_value("true", DataFieldType::Boolean).unwrap();
        assert_eq!(val, serde_json::Value::Bool(true));
    }

    #[test]
    fn parse_default_value_array() {
        let val = parse_default_value("[]", DataFieldType::Array).unwrap();
        assert_eq!(val, serde_json::json!([]));
    }

    #[test]
    fn parse_default_value_invalid_number() {
        let err = parse_default_value("abc", DataFieldType::Number);
        assert!(err.is_err());
        let msg = err.unwrap_err().to_string();
        assert!(
            msg.contains("cannot parse default"),
            "error should explain: {}",
            msg
        );
    }

    #[test]
    fn parse_default_value_object() {
        let val = parse_default_value("{}", DataFieldType::Object).unwrap();
        assert_eq!(val, serde_json::json!({}));
    }

    #[test]
    fn parse_default_value_type_mismatch() {
        let err = parse_default_value("[]", DataFieldType::Object);
        assert!(err.is_err());
        let msg = err.unwrap_err().to_string();
        assert!(msg.contains("expected object"), "error: {}", msg);

        let err = parse_default_value("{}", DataFieldType::Array);
        assert!(err.is_err());
        let msg = err.unwrap_err().to_string();
        assert!(msg.contains("expected array"), "error: {}", msg);
    }

    #[test]
    fn migrate_dry_run_prune_no_mutation() {
        let (mut store, _dir) = setup_store(migrate_schema());
        store.data.entries.insert(
            "projects".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "abc1".to_string(),
                    value: "proj-a".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: Some(serde_json::json!({
                        "status": "active",
                        "tags": ["rust"],
                        "priority": 5,
                        "obsolete": "should stay in dry run"
                    })),
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("projects", true, true).unwrap();
        assert_eq!(result.modified, 1, "dry run should report modifications");
        assert!(
            result.changes[0]
                .fields_pruned
                .contains(&"obsolete".to_string()),
            "should report obsolete as pruned"
        );

        // Verify no actual mutation
        if let DataValue::History { entries, .. } = &store.data.entries["projects"] {
            let data = entries[0].data.as_ref().unwrap();
            assert_eq!(
                data["obsolete"],
                serde_json::json!("should stay in dry run"),
                "dry run should not prune"
            );
        } else {
            panic!("expected history");
        }
    }

    #[test]
    fn migrate_dry_run_data_none_no_mutation() {
        let (mut store, _dir) = setup_store(migrate_schema());
        store.data.entries.insert(
            "projects".to_string(),
            DataValue::History {
                entries: vec![HistoryEntry {
                    index: 1,
                    id: "abc1".to_string(),
                    value: "proj-a".to_string(),
                    ts: "2026-01-01T00:00:00Z".to_string(),
                    data: None,
                    memory: None,
                }],
                memory: None,
            },
        );

        let result = store.migrate("projects", false, true).unwrap();
        assert_eq!(result.modified, 1, "dry run should report modifications");
        assert!(!result.changes[0].fields_added.is_empty());

        // Verify data slot is still None
        if let DataValue::History { entries, .. } = &store.data.entries["projects"] {
            assert!(
                entries[0].data.is_none(),
                "dry run should not create data blob"
            );
        } else {
            panic!("expected history");
        }
    }

    // -----------------------------------------------------------------------
    // update_entry tests
    // -----------------------------------------------------------------------

    fn test_schema_with_data() -> &'static str {
        r#"
[keys.items]
type = "history"

[keys.items.data]
status = { type = "string", required = true }
count = { type = "number" }

[keys.shelf]
type = "list"

[keys.shelf.data]
status = { type = "string", required = true }
count = { type = "number" }
"#
    }

    fn get_history_entry(store: &KvStore, key: &str, index: u64) -> HistoryEntry {
        match store.get(key).unwrap() {
            DataValue::History { entries, .. } => entries
                .iter()
                .find(|e| e.index == index)
                .cloned()
                .expect("entry not found"),
            _ => panic!("expected history"),
        }
    }

    fn get_list_entry(store: &KvStore, key: &str, index: u64) -> ListEntry {
        match store.get(key).unwrap() {
            DataValue::List { items, .. } => items
                .iter()
                .find(|e| e.index == index)
                .cloned()
                .expect("entry not found"),
            _ => panic!("expected list"),
        }
    }

    #[test]
    fn update_value_only_preserves_ts_and_id_history() {
        let (mut store, _dir) = setup_store(test_schema());
        let push = store.push("flavor_history", "mint", None, None).unwrap();
        let before = get_history_entry(&store, "flavor_history", push.index);

        let result = store
            .update_entry(
                "flavor_history",
                &IdRef::Index(push.index),
                Some("peppermint"),
                None,
            )
            .unwrap();
        let after = get_history_entry(&store, "flavor_history", push.index);

        assert_eq!(result.index, push.index);
        assert_eq!(result.id, push.id);
        assert_eq!(after.value, "peppermint");
        assert_eq!(after.ts, before.ts, "ts must be preserved");
        assert_eq!(after.id, before.id, "id must be preserved");
        assert_eq!(after.data, before.data, "data must be unchanged");
    }

    #[test]
    fn update_value_only_preserves_ts_and_id_list() {
        let (mut store, _dir) = setup_store(test_schema());
        let push = store.push("tags", "alpha", None, None).unwrap();
        let before = get_list_entry(&store, "tags", push.index);

        let result = store
            .update_entry("tags", &IdRef::Index(push.index), Some("beta"), None)
            .unwrap();
        let after = get_list_entry(&store, "tags", push.index);

        assert_eq!(result.index, push.index);
        assert_eq!(result.id, push.id);
        assert_eq!(after.value, "beta");
        assert_eq!(after.ts, before.ts, "ts must be preserved");
        assert_eq!(after.id, before.id, "id must be preserved");
        assert_eq!(after.data, before.data, "data must be unchanged");
    }

    #[test]
    fn update_data_merges_with_existing_history() {
        let (mut store, _dir) = setup_store(test_schema());
        let initial_data = serde_json::json!({"a": 1, "b": 2});
        let push = store
            .push("flavor_history", "chai", Some(initial_data), None)
            .unwrap();
        let before = get_history_entry(&store, "flavor_history", push.index);

        let patch = serde_json::json!({"b": 3});
        store
            .update_entry(
                "flavor_history",
                &IdRef::Index(push.index),
                None,
                Some(patch),
            )
            .unwrap();
        let after = get_history_entry(&store, "flavor_history", push.index);

        let data = after.data.as_ref().unwrap();
        assert_eq!(data["a"], serde_json::json!(1), "a must be unchanged");
        assert_eq!(data["b"], serde_json::json!(3), "b must be updated");
        assert_eq!(after.ts, before.ts, "ts must be preserved");
        assert_eq!(after.id, before.id, "id must be preserved");
        assert_eq!(after.value, before.value, "value must be unchanged");
    }

    #[test]
    fn update_data_merges_with_existing_list() {
        let (mut store, _dir) = setup_store(test_schema());
        let initial_data = serde_json::json!({"a": 1, "b": 2});
        let push = store
            .push("tags", "item", Some(initial_data), None)
            .unwrap();
        let before = get_list_entry(&store, "tags", push.index);

        let patch = serde_json::json!({"b": 3});
        store
            .update_entry("tags", &IdRef::Index(push.index), None, Some(patch))
            .unwrap();
        let after = get_list_entry(&store, "tags", push.index);

        let data = after.data.as_ref().unwrap();
        assert_eq!(data["a"], serde_json::json!(1), "a must be unchanged");
        assert_eq!(data["b"], serde_json::json!(3), "b must be updated");
        assert_eq!(after.ts, before.ts, "ts must be preserved");
        assert_eq!(after.id, before.id, "id must be preserved");
        assert_eq!(after.value, before.value, "value must be unchanged");
    }

    #[test]
    fn update_value_and_data_together() {
        let (mut store, _dir) = setup_store(test_schema());
        let initial_data = serde_json::json!({"x": 10});
        let push = store
            .push("flavor_history", "original", Some(initial_data), None)
            .unwrap();

        let patch = serde_json::json!({"x": 99});
        store
            .update_entry(
                "flavor_history",
                &IdRef::Index(push.index),
                Some("updated"),
                Some(patch),
            )
            .unwrap();
        let after = get_history_entry(&store, "flavor_history", push.index);

        assert_eq!(after.value, "updated");
        let data = after.data.as_ref().unwrap();
        assert_eq!(data["x"], serde_json::json!(99));
    }

    #[test]
    fn update_data_null_deletes_field() {
        let (mut store, _dir) = setup_store(test_schema());
        let initial_data = serde_json::json!({"a": 1, "b": 2});
        let push = store
            .push("flavor_history", "rooibos", Some(initial_data), None)
            .unwrap();

        let patch = serde_json::json!({"b": null});
        store
            .update_entry(
                "flavor_history",
                &IdRef::Index(push.index),
                None,
                Some(patch),
            )
            .unwrap();
        let after = get_history_entry(&store, "flavor_history", push.index);

        let data = after.data.as_ref().unwrap();
        assert_eq!(data["a"], serde_json::json!(1), "a must remain");
        assert!(
            data.get("b").is_none(),
            "b must be deleted, not set to null"
        );
    }

    #[test]
    fn update_data_on_entry_with_no_data() {
        let (mut store, _dir) = setup_store(test_schema());
        let push = store.push("flavor_history", "oolong", None, None).unwrap();

        let patch = serde_json::json!({"a": 1});
        store
            .update_entry(
                "flavor_history",
                &IdRef::Index(push.index),
                None,
                Some(patch),
            )
            .unwrap();
        let after = get_history_entry(&store, "flavor_history", push.index);

        let data = after.data.as_ref().expect("data must be Some after update");
        assert_eq!(data["a"], serde_json::json!(1));
    }

    #[test]
    fn update_data_validation_rejects_unknown_field() {
        let (mut store, _dir) = setup_store(test_schema_with_data());
        let initial_data = serde_json::json!({"status": "open"});
        let push = store
            .push("items", "task-1", Some(initial_data), None)
            .unwrap();
        let before = get_history_entry(&store, "items", push.index);

        let patch = serde_json::json!({"unknown_field": "bad"});
        let err = store
            .update_entry("items", &IdRef::Index(push.index), None, Some(patch))
            .unwrap_err();

        assert!(
            matches!(err, KvError::DataValidation { .. }),
            "expected DataValidation, got {:?}",
            err
        );
        let after = get_history_entry(&store, "items", push.index);
        assert_eq!(
            after.data, before.data,
            "entry must be unchanged on failure"
        );
    }

    #[test]
    fn update_data_validation_rejects_type_mismatch() {
        let (mut store, _dir) = setup_store(test_schema_with_data());
        let initial_data = serde_json::json!({"status": "open"});
        let push = store
            .push("items", "task-2", Some(initial_data), None)
            .unwrap();
        let before = get_history_entry(&store, "items", push.index);

        // count is type=number; passing a string triggers type mismatch
        let patch = serde_json::json!({"count": "not-a-number"});
        let err = store
            .update_entry("items", &IdRef::Index(push.index), None, Some(patch))
            .unwrap_err();

        assert!(
            matches!(err, KvError::DataValidation { .. }),
            "expected DataValidation, got {:?}",
            err
        );
        let after = get_history_entry(&store, "items", push.index);
        assert_eq!(
            after.data, before.data,
            "entry must be unchanged on failure"
        );
    }

    #[test]
    fn update_data_validation_keeps_required_field_via_merge() {
        let (mut store, _dir) = setup_store(test_schema_with_data());
        // Push with status (required) and count (optional)
        let initial_data = serde_json::json!({"status": "open", "count": 1});
        let push = store
            .push("items", "task-3", Some(initial_data), None)
            .unwrap();

        // Patch only updates count — status is not in patch but is satisfied by existing data
        let patch = serde_json::json!({"count": 2});
        store
            .update_entry("items", &IdRef::Index(push.index), None, Some(patch))
            .unwrap();
        let after = get_history_entry(&store, "items", push.index);

        let data = after.data.as_ref().unwrap();
        assert_eq!(
            data["status"],
            serde_json::json!("open"),
            "required field preserved in merge"
        );
        assert_eq!(data["count"], serde_json::json!(2), "count updated");
    }

    #[test]
    fn update_entry_not_found_by_index() {
        let (mut store, _dir) = setup_store(test_schema());
        store
            .push("flavor_history", "darjeeling", None, None)
            .unwrap();

        let err = store
            .update_entry("flavor_history", &IdRef::Index(999), Some("new"), None)
            .unwrap_err();
        assert!(
            matches!(err, KvError::EntryNotFound { .. }),
            "expected EntryNotFound, got {:?}",
            err
        );
    }

    #[test]
    fn update_entry_not_found_by_id_prefix() {
        let (mut store, _dir) = setup_store(test_schema());
        store.push("flavor_history", "sencha", None, None).unwrap();

        let err = store
            .update_entry(
                "flavor_history",
                &IdRef::Id("nonexistentprefix".to_string()),
                Some("new"),
                None,
            )
            .unwrap_err();
        assert!(
            matches!(err, KvError::EntryNotFound { .. }),
            "expected EntryNotFound, got {:?}",
            err
        );
    }

    #[test]
    fn update_ambiguous_id_prefix() {
        let (mut store, _tmp) = setup_store(test_schema());
        store.push("tags", "alpha", None, None).unwrap();
        store.push("tags", "beta", None, None).unwrap();
        // Empty prefix matches every entry — always ambiguous when there are 2+.
        let err = store
            .update_entry(
                "tags",
                &IdRef::Id("".to_string()),
                Some("beta-updated"),
                None,
            )
            .unwrap_err();
        match err {
            KvError::AmbiguousId { count, .. } => assert_eq!(count, 2),
            other => panic!("expected AmbiguousId, got {:?}", other),
        }
    }

    #[test]
    fn update_wrong_key_type_counter() {
        let (mut store, _dir) = setup_store(test_schema());
        let err = store
            .update_entry("warmth", &IdRef::Index(0), Some("5"), None)
            .unwrap_err();
        assert!(
            matches!(err, KvError::TypeMismatch { .. }),
            "expected TypeMismatch, got {:?}",
            err
        );
    }

    #[test]
    fn update_id_lookup_by_prefix_unique() {
        let (mut store, _dir) = setup_store(test_schema());
        let push = store.push("flavor_history", "gyokuro", None, None).unwrap();

        // Take first 4 chars of id as prefix
        let prefix = push.id[..4.min(push.id.len())].to_string();

        let result = store
            .update_entry(
                "flavor_history",
                &IdRef::Id(prefix),
                Some("gyokuro-updated"),
                None,
            )
            .unwrap();

        assert_eq!(result.index, push.index);
        assert_eq!(result.id, push.id);
        let after = get_history_entry(&store, "flavor_history", push.index);
        assert_eq!(after.value, "gyokuro-updated");
    }

    // -- rename_key --

    #[test]
    fn rename_key_happy_path() {
        let (mut store, _dir) = setup_store(test_schema());

        // Push some data into flavor_history so there is something to rename
        let push1 = store.push("flavor_history", "matcha", None, None).unwrap();
        let push2 = store.push("flavor_history", "hojicha", None, None).unwrap();
        store.save().unwrap();

        store.rename_key("flavor_history", "tea_history").unwrap();

        // Old key gone from schema and data
        assert!(!store.schema.keys.contains_key("flavor_history"));
        assert!(!store.data.entries.contains_key("flavor_history"));

        // New key present in schema and data
        assert!(store.schema.keys.contains_key("tea_history"));
        assert!(store.data.entries.contains_key("tea_history"));

        // Data preserved: IDs and values intact (newest entry first)
        let entries = match store.data.entries.get("tea_history").unwrap() {
            DataValue::History { entries, .. } => entries,
            _ => panic!("Expected History"),
        };
        assert_eq!(entries.len(), 2);
        assert_eq!(entries[0].value, "hojicha");
        assert_eq!(entries[0].id, push2.id);
        assert_eq!(entries[1].value, "matcha");
        assert_eq!(entries[1].id, push1.id);
    }

    #[test]
    fn rename_key_old_key_not_found() {
        let (mut store, _dir) = setup_store(test_schema());

        let err = store.rename_key("nonexistent", "something").unwrap_err();
        assert!(
            matches!(err, KvError::KeyNotFound(ref k) if k == "nonexistent"),
            "Expected KeyNotFound, got: {err}"
        );
    }

    #[test]
    fn rename_key_new_key_already_exists() {
        let (mut store, _dir) = setup_store(test_schema());

        let err = store.rename_key("warmth", "capped").unwrap_err();
        assert!(
            matches!(err, KvError::DataValidation { .. }),
            "Expected DataValidation, got: {err}"
        );
        assert!(err.to_string().contains("already exists"));
    }

    #[test]
    fn rename_key_invalid_new_key_name() {
        let (mut store, _dir) = setup_store(test_schema());

        // Dots are rejected
        let err = store.rename_key("warmth", "bad.name").unwrap_err();
        assert!(
            matches!(err, KvError::DataValidation { .. }),
            "Expected DataValidation, got: {err}"
        );

        // Empty name rejected
        let err = store.rename_key("warmth", "").unwrap_err();
        assert!(
            matches!(err, KvError::DataValidation { .. }),
            "Expected DataValidation, got: {err}"
        );

        // Special chars rejected
        let err = store.rename_key("warmth", "no spaces!").unwrap_err();
        assert!(
            matches!(err, KvError::DataValidation { .. }),
            "Expected DataValidation, got: {err}"
        );
    }

    #[test]
    fn rename_key_with_no_data_entries() {
        // Key exists in schema but has no data entries yet
        let (mut store, _dir) = setup_store(test_schema());

        // warmth is a counter with no data pushed yet -- but counters get a
        // default. Use a fresh history key via add_key_to_schema instead.
        store
            .add_key_to_schema("empty_history", "history", None)
            .unwrap();
        assert!(!store.data.entries.contains_key("empty_history"));

        store
            .rename_key("empty_history", "renamed_history")
            .unwrap();

        assert!(!store.schema.keys.contains_key("empty_history"));
        assert!(store.schema.keys.contains_key("renamed_history"));
        // No crash, no data entry created out of thin air
        assert!(!store.data.entries.contains_key("empty_history"));
    }

    #[test]
    fn rename_key_data_preservation() {
        let (mut store, _dir) = setup_store(test_schema());

        // Push entries with data payloads
        let data_json = serde_json::json!({"mood": "calm"});
        store
            .push("flavor_history", "sencha", Some(data_json.clone()), None)
            .unwrap();
        store.save().unwrap();

        // Serialize before rename
        let before =
            serde_json::to_string(store.data.entries.get("flavor_history").unwrap()).unwrap();

        store.rename_key("flavor_history", "tea_log").unwrap();

        // Serialize after rename -- should be identical
        let after = serde_json::to_string(store.data.entries.get("tea_log").unwrap()).unwrap();

        assert_eq!(
            before, after,
            "Serialized data should be identical after rename"
        );
    }
}