differential-engine 0.13.0

//! Configuration, split by ownership (ADR 0012, amended by ADR 0018-era split):
//!
//! - **Repo-level** `.differential.toml` at the target repo's root —
//!   classification hints only. Shared by everyone reviewing the repo.
//! - **User-level** `~/.config/differential/config.toml` (XDG) — `[grouping]`:
//!   which agent CLI to run and its timeout, and `[review]`: which palette the
//!   reviewer wears, how much context it shows around a hunk, and which diff
//!   layout it opens in, and `[keys]`: which keys the reviewer's actions answer
//!   to. All per-user choices, not properties of the repo, so none of them
//!   lives in it.
//!
//! HARD RULE (ADR 0012): config tunes classification hints and tool behaviour.
//! It can never remove a file or hunk from enumeration — enumeration runs before
//! and independently of anything in this module, and nothing here is consulted
//! by the parser or the invariants.

use std::collections::BTreeMap;
use std::path::{Path, PathBuf};

use globset::{Glob, GlobSet, GlobSetBuilder};
use serde::{Deserialize, Serialize};

use crate::EngineError;

pub const CONFIG_FILE_NAME: &str = ".differential.toml";
pub const USER_CONFIG_DIR: &str = "differential";
pub const USER_CONFIG_FILE_NAME: &str = "config.toml";

#[derive(Debug, Default, Deserialize)]
#[serde(deny_unknown_fields)]
struct RawConfig {
    #[serde(default)]
    classify: RawClassify,
    /// Rejected with a migration hint — [grouping] moved to the user config.
    #[serde(default)]
    grouping: Option<toml::Table>,
    // Reserved for later milestones; accepted so the file format is stable.
    // `IgnoredAny` says exactly that — the table is parsed and discarded,
    // where a `toml::Table` was allocated in full and then discarded, with a
    // `let _ =` further down whose only job was to quiet the compiler about
    // a field nothing reads.
    //
    // Named with a leading underscore because nothing reads them and nothing
    // should: the `#[serde(rename)]` keeps the file's own spelling.
    #[serde(default, rename = "ordering")]
    _ordering: serde::de::IgnoredAny,
    #[serde(default, rename = "stack")]
    _stack: serde::de::IgnoredAny,
}

/// The user-level file: `[grouping]`, `[review]` and `[keys]`.
#[derive(Debug, Default, Deserialize)]
#[serde(deny_unknown_fields)]
struct RawUserConfig {
    #[serde(default)]
    grouping: GroupingConfig,
    #[serde(default)]
    review: ReviewConfig,
    #[serde(default)]
    keys: KeysConfig,
}

/// Everything `parse_user` reads, so `load` assigns one value rather than
/// growing a second assignment every time the user file gains a table.
///
/// Serialisable, because the reviewer's config modal writes it back whole
/// ([`Config::save_user`]).
#[derive(Debug, Clone, Default, PartialEq, Serialize)]
pub struct UserConfig {
    pub grouping: GroupingConfig,
    pub review: ReviewConfig,
    #[serde(skip_serializing_if = "KeysConfig::is_empty")]
    pub keys: KeysConfig,
}

/// Which agent to run, by name.
///
/// It used to be a free argv, and that was the wrong shape. The grouping stage
/// does not merely spawn a process: it hands the agent a tool allowlist, a
/// fetch command and a prompt written for what that agent can do (ADR 0022).
/// An arbitrary argv gets the prompt and none of the rest, so it was a knob
/// that looked like it worked. A name selects an invocation this crate builds
/// whole, and adding an agent is adding a variant here.
///
/// The name also answers what a reviewer is shown while they wait — the argv
/// never could, at four times the width of the line it had.
///
/// **Four of the five keep the model read-only; `Pi` does not** (ADR 0033).
/// Read [`Agent::read_only`] and [`ReadOnly::is_enforced`] before choosing one.
#[derive(
    Debug,
    Clone,
    Copy,
    Default,
    PartialEq,
    Eq,
    Deserialize,
    Serialize,
    strum::IntoStaticStr,
    strum::VariantArray,
)]
#[serde(rename_all = "kebab-case")]
#[strum(serialize_all = "kebab-case")]
pub enum Agent {
    /// Headless `claude`, read-only by tool allowlist (ADR 0022).
    #[default]
    ClaudeCode,
    /// Headless `codex exec`, read-only by OS sandbox (Seatbelt, bubblewrap).
    Codex,
    /// Headless `droid exec`, read-only by default — the tier is what we do
    /// not pass.
    Droid,
    /// Headless `copilot`, read-only by tool allowlist and an explicit deny.
    Copilot,
    /// Headless `pi`, **read-only is NOT enforced** (ADR 0033).
    ///
    /// Pi ships no sandbox and no per-command allowlist, and its `-t` flag
    /// toggles whole tools. The model needs `bash` to run the fetch command
    /// and `git diff`, and `bash` also lets it write, commit and push. Nothing
    /// but the prompt stops it. Choose this agent only knowing that.
    Pi,
}

impl Agent {
    /// Every variant, in declaration order, from strum's `VariantArray`: the
    /// derive is what keeps the list whole, so there is no hand-kept array to
    /// forget a variant in.
    pub const ALL: &'static [Agent] = <Agent as strum::VariantArray>::VARIANTS;

    /// The name this answers to in the config file. strum's `IntoStaticStr`,
    /// renamed as serde renames it; `every_*_name_round_trips` in this module
    /// pins the two derives to the same spelling.
    pub fn key(self) -> &'static str {
        self.into()
    }

    /// Whether anyone has ever run this agent's command line.
    ///
    /// Not a quality judgement — a claim about provenance, and the only honest
    /// one this crate can make. Every argv here is written from its agent's
    /// documentation, and a test can assert the string this crate builds but
    /// never that the CLI on the other end accepts it. CI cannot either: the
    /// binary is not installed and its flags move between releases.
    ///
    /// `true` means `dfr agents --probe` passed all four checks against the
    /// real CLI, and the argv is in this repository because of that run.
    ///
    /// **`false` means likely wrong, not merely unchecked.** Of the three
    /// checked so far, two were broken: Claude Code's allowlist did not bind
    /// without `--permission-mode default`, and Codex was passing
    /// `--ask-for-approval`, which its `exec` subcommand rejects outright. Both
    /// came from documentation that was accurate about the product and wrong
    /// about the entry point. Nothing suggests the unchecked two are better.
    ///
    /// A caller that offers a user this list must say so, for the same reason
    /// it must say what [`Agent::read_only`] answers: the person choosing is
    /// the person who carries it.
    ///
    /// This flips when someone runs the probe and the argv lands — never
    /// because it looks right.
    pub fn proven(self) -> bool {
        match self {
            // Probed on a real call: all four checks passed.
            Agent::ClaudeCode | Agent::Codex | Agent::Pi => true,
            // Written from documentation. Droid needs a paid Factory plan and
            // Copilot a Copilot seat, so neither has been run.
            Agent::Droid | Agent::Copilot => false,
        }
    }

    /// What stops this agent writing, if anything.
    ///
    /// A caller that shows a user the list of agents MUST show this too. The
    /// person picking a name is the person who needs to know, and exactly one
    /// answer here is [`ReadOnly::NotEnforced`].
    pub fn read_only(self) -> ReadOnly {
        match self {
            Agent::ClaudeCode | Agent::Copilot => ReadOnly::ToolAllowlist,
            Agent::Codex => ReadOnly::OsSandbox,
            Agent::Droid => ReadOnly::AgentDefault,
            Agent::Pi => ReadOnly::NotEnforced,
        }
    }
}

/// What keeps an agent from writing.
///
/// An enum rather than a `bool` plus a sentence, because the three enforcing
/// answers are not interchangeable and a reader deciding whether to trust one
/// needs to know which they have. An OS sandbox holds against a model that
/// tries; an allowlist holds against a model that asks; a default holds only
/// until someone adds a flag.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ReadOnly {
    /// The agent may run only the tools it was given, and writing is not one.
    ToolAllowlist,
    /// The agent may run anything and the kernel refuses the writes.
    OsSandbox,
    /// The agent is read-only until told otherwise, and it is not told.
    AgentDefault,
    /// **Nothing stops it.** The agent can write, commit and push, and only the
    /// prompt asks it not to. See [`Agent::Pi`] and ADR 0033 for why one agent
    /// is here and why that was a choice rather than an oversight.
    NotEnforced,
}

impl ReadOnly {
    pub fn is_enforced(self) -> bool {
        !matches!(self, ReadOnly::NotEnforced)
    }
}

/// `[grouping]` — pure data; the application layer turns it into an LLM
/// backend (ADR 0018, 0020).
#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct GroupingConfig {
    /// Which agent runs the grouping call. Default: `claude-code`.
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub agent: Option<Agent>,
    /// How long to wait for it. Default: [`DEFAULT_TIMEOUT_SECS`].
    ///
    /// This one stays a number because it tunes the agent rather than replacing
    /// it: a slow machine or a large change may genuinely need longer.
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub timeout_secs: Option<u64>,
}

/// How long a grouping call may run when `[grouping].timeout_secs` is unset.
/// Every agent's backend starts from it.
pub const DEFAULT_TIMEOUT_SECS: u64 = 1200;

/// Which palette the terminal reviewer wears, by name.
///
/// A name, for the same reason [`Agent`] is one: a palette is not a colour the
/// caller supplies but a whole coherent set the renderer builds — thirty-one
/// fields plus the syntax theme the code itself is painted with, all derived
/// together so the chrome and the code cannot disagree (ADR 0024). A free-form
/// colour list would be a knob that looked like it worked.
///
/// Adding a theme is adding a variant here and a seed in the renderer.
#[derive(
    Debug,
    Clone,
    Copy,
    Default,
    PartialEq,
    Eq,
    Deserialize,
    Serialize,
    strum::IntoStaticStr,
    strum::VariantArray,
)]
#[serde(rename_all = "kebab-case")]
#[strum(serialize_all = "kebab-case")]
pub enum ThemeName {
    /// The original palette: a dark slate ground with a cyan accent.
    #[default]
    Dark,
    OneDark,
    OneLight,
    GruvboxDark,
    GruvboxLight,
    SolarizedDark,
    SolarizedLight,
    CatppuccinMocha,
    CatppuccinLatte,
    Dracula,
    Monokai,
}

/// `[review]` — how the terminal reviewer looks, and how much of a file it
/// shows around a hunk.
///
/// Presentation only: it can widen what is *displayed* around a hunk and can
/// never change which hunks exist. Enumeration is total and runs before any of
/// this (ADR 0005, 0012).
#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct ReviewConfig {
    /// Context lines shown either side of a hunk before any expansion.
    #[serde(default = "default_context")]
    pub context: usize,
    /// Lines one `z` at a context boundary row pulls in.
    #[serde(default = "default_context_step")]
    pub context_step: usize,
    /// Which diff layout a review opens in, before the reader says otherwise.
    ///
    /// A DEFAULT, not a setting: `s` still toggles, and the toggle is recorded
    /// per review. A review that has recorded a choice keeps it whatever this
    /// says, so changing it never moves a layout under someone mid-read.
    #[serde(default)]
    pub diff: DiffLayout,
    /// Which palette to wear. Default: `dark`.
    #[serde(default)]
    pub theme: ThemeName,
    /// The command that opens a file at a line when the reader presses `e`.
    ///
    /// `{file}` and `{line}` say where the path and the line go. A command
    /// naming neither gets the path appended and opens the file at the top.
    /// Unset falls back to `$VISUAL` and then `$EDITOR`, which the application
    /// layer reads — the environment is an adapter's to touch, not this
    /// module's (ADR 0038).
    #[serde(default)]
    pub editor: Option<String>,
}

/// How the reviewer lays a hunk out.
///
/// An enum rather than a bool because a config key is permanent, and a third
/// layout would otherwise need a second key contradicting the first.
#[derive(
    Debug,
    Clone,
    Copy,
    PartialEq,
    Eq,
    Default,
    Deserialize,
    Serialize,
    strum::IntoStaticStr,
    strum::VariantArray,
)]
#[serde(rename_all = "lowercase")]
#[strum(serialize_all = "lowercase")]
pub enum DiffLayout {
    /// Old and new side by side.
    #[default]
    Split,
    /// One column, removals above additions.
    Unified,
}

impl DiffLayout {
    /// Every variant, in declaration order, from strum's `VariantArray`: the
    /// derive is what keeps the list whole, so there is no hand-kept array to
    /// forget a variant in.
    pub const ALL: &'static [DiffLayout] = <DiffLayout as strum::VariantArray>::VARIANTS;

    pub fn is_split(self) -> bool {
        matches!(self, DiffLayout::Split)
    }

    /// The name this answers to in the config file. strum's `IntoStaticStr`,
    /// renamed as serde renames it; `every_*_name_round_trips` in this module
    /// pins the two derives to the same spelling.
    pub fn key(self) -> &'static str {
        self.into()
    }
}

impl ThemeName {
    /// Every variant, in declaration order, from strum's `VariantArray`: the
    /// derive is what keeps the list whole, so there is no hand-kept array to
    /// forget a variant in.
    pub const ALL: &'static [ThemeName] = <ThemeName as strum::VariantArray>::VARIANTS;

    /// The name this answers to in the config file. strum's `IntoStaticStr`,
    /// renamed as serde renames it; `every_*_name_round_trips` in this module
    /// pins the two derives to the same spelling.
    pub fn key(self) -> &'static str {
        self.into()
    }
}

/// The placeholder standing for the path to open.
pub const EDITOR_FILE: &str = "{file}";
/// The placeholder standing for the line to open it at.
pub const EDITOR_LINE: &str = "{line}";

/// A parsed `[review].editor` — the command that opens a file at a line.
///
/// **A command, not a name**, and the only key in this module that is. `agent`
/// and `theme` are names because the invocation behind each carries more than
/// an argv: an agent is handed a tool allowlist and a prompt written for what
/// it can do (ADR 0022, 0033), and a palette is thirty-odd values derived
/// together so the chrome and the code cannot disagree (ADR 0024). In both, a
/// free-form value would have been a knob that looked like it worked.
///
/// Neither reason reaches an editor. The invocation carries a path and a line
/// and nothing else, every editor spells the line differently, and a name
/// would have frozen the list of editors a reader may use into an enum in this
/// crate. So the reader writes the command (ADR 0038).
///
/// Parsed once, at load, so a command that cannot be split is an error the
/// reader sees when they start rather than when they press the key.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct EditorCommand {
    argv: Vec<String>,
    carries_line: bool,
    carries_file: bool,
}

impl EditorCommand {
    /// Split the command into words. `origin` names the file or the variable
    /// it came from, for the error.
    ///
    /// `shlex` does the splitting rather than a hand-rolled scanner (design
    /// rule 5): quoting is the whole of the problem here, and a path with a
    /// space in it is the case that would have found a hand-rolled bug.
    pub fn parse(text: &str, origin: &str) -> Result<EditorCommand, EngineError> {
        let fail = |msg: String| EngineError::Config {
            path: origin.to_string(),
            msg,
        };
        let argv = shlex::split(text).ok_or_else(|| {
            fail(format!(
                "editor command does not split into words \
                 (an unbalanced quote?): {text:?}"
            ))
        })?;
        if argv.is_empty() {
            return Err(fail("editor command is empty".to_string()));
        }
        // The first word is the program, and a placeholder there would make
        // the program the FILE. On a source file carrying the executable bit
        // that is not a failed spawn to shrug at — it is `e` running the file
        // under the cursor. Caught here, where every other malformed value is.
        if argv[0].contains(EDITOR_FILE) || argv[0].contains(EDITOR_LINE) {
            return Err(fail(format!(
                "the first word is the program to run, and it may not be a \
                 placeholder: {:?}",
                argv[0]
            )));
        }
        Ok(EditorCommand {
            carries_line: argv.iter().any(|w| w.contains(EDITOR_LINE)),
            carries_file: argv.iter().any(|w| w.contains(EDITOR_FILE)),
            argv,
        })
    }

    /// The argv that opens `file` at `line`.
    ///
    /// A command naming no `{file}` gets the path appended, which is what
    /// makes a bare `$EDITOR` work; it opens the file at the top, and
    /// [`carries_line`](Self::carries_line) is what lets the caller say so.
    ///
    /// **`{line}` is substituted before `{file}`.** A path holding the literal
    /// text `{line}` would otherwise be read as a placeholder by the second
    /// pass. `str::replace` never re-scans what it inserts, so one order is
    /// all it takes.
    ///
    /// The path is rendered lossily, as every path in the renderer above this
    /// already is — `schema::FileEntry::path` is a `String`.
    ///
    /// Substitution reaches every word but the first, which [`parse`](Self::parse)
    /// has already refused to let hold a placeholder.
    pub fn argv(&self, file: &Path, line: u32) -> Vec<String> {
        let path = file.to_string_lossy();
        let line = line.to_string();
        let mut argv: Vec<String> = self
            .argv
            .iter()
            .map(|w| w.replace(EDITOR_LINE, &line).replace(EDITOR_FILE, &path))
            .collect();
        if !self.carries_file {
            argv.push(path.into_owned());
        }
        argv
    }

    /// Whether the command says where the line goes. `false` means the editor
    /// opens the file at the top, which the reader is owed a word about.
    pub fn carries_line(&self) -> bool {
        self.carries_line
    }

    /// The program, for an error message that names what failed.
    pub fn program(&self) -> &str {
        &self.argv[0]
    }
}

const fn default_context() -> usize {
    3
}

const fn default_context_step() -> usize {
    10
}

impl Default for ReviewConfig {
    fn default() -> Self {
        ReviewConfig {
            context: default_context(),
            context_step: default_context_step(),
            diff: DiffLayout::default(),
            theme: ThemeName::default(),
            editor: None,
        }
    }
}

/// Something the terminal reviewer does on a key, by name (ADR 0036).
///
/// A name rather than a key, because a key is the reader's to choose and the
/// thing it does is not. `[keys]` maps these to key strings; the renderer owns
/// what a key string means and what the defaults are, so this crate never
/// learns a terminal's vocabulary. One name means one thing wherever it
/// works: `down` moves in the plan pane, the file list and the findings list
/// alike, so a reader binds it once.
///
/// Adding an action is adding a variant here, its name in [`Action::key`],
/// and its default keys and its arm in the renderer.
#[derive(
    Debug,
    Clone,
    Copy,
    PartialEq,
    Eq,
    PartialOrd,
    Ord,
    Hash,
    Deserialize,
    Serialize,
    strum::IntoStaticStr,
    strum::VariantArray,
)]
#[serde(rename_all = "kebab-case")]
#[strum(serialize_all = "kebab-case")]
pub enum Action {
    ToggleFocus,
    Open,
    Close,
    Down,
    Up,
    NextGroup,
    PrevGroup,
    HalfPageDown,
    HalfPageUp,
    Top,
    Bottom,
    NextHunk,
    PrevHunk,
    ToggleSplit,
    ToggleWrap,
    ShiftRight,
    ShiftLeft,
    ShiftReset,
    GrowDiff,
    ShrinkDiff,
    Fold,
    Files,
    /// Hand the terminal to the reader's own editor, on the line under the
    /// cursor. The command it runs is `[review].editor` (ADR 0038); this is
    /// only the key that asks for it.
    ExternalEditor,
    Findings,
    Search,
    ToggleReviewed,
    Select,
    Comment,
    Delete,
    ClearNotes,
    Copy,
    Reply,
    Resolve,
    Refetch,
    Publish,
    Back,
}

impl Action {
    /// Every variant, in declaration order, from strum's `VariantArray`: the
    /// derive is what keeps the list whole, so there is no hand-kept array to
    /// forget a variant in.
    pub const ALL: &'static [Action] = <Action as strum::VariantArray>::VARIANTS;

    /// The name this answers to in the config file. strum's `IntoStaticStr`,
    /// renamed as serde renames it; `every_*_name_round_trips` in this module
    /// pins the two derives to the same spelling.
    pub fn key(self) -> &'static str {
        self.into()
    }
}

/// `[keys]` — which keys an action answers to, as the reader wrote them.
///
/// An action named here takes EXACTLY these keys, in every place it works:
/// its defaults are replaced, not extended, so `[]` unbinds it. An action not
/// named keeps its defaults.
///
/// The strings stay strings in this crate. What `"ctrl-d"` means, whether it
/// parses, and whether two actions now share a key are the renderer's
/// questions, answered by one library call before a terminal is touched
/// (ADR 0036).
#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize, Serialize)]
#[serde(transparent)]
pub struct KeysConfig(pub BTreeMap<Action, Vec<String>>);

impl KeysConfig {
    /// No action rebound: the file needs no `[keys]` table at all.
    pub fn is_empty(&self) -> bool {
        self.0.is_empty()
    }

    /// One action's keys as `[keys]` writes them: `["ctrl-j", "d d"]`. The
    /// config modal shows and edits a row in exactly the file's syntax, so
    /// nothing a reader types there means something else in the file.
    pub fn render_list(keys: &[String]) -> String {
        let list = keys.iter().cloned().map(toml::Value::String).collect();
        toml::Value::Array(list).to_string()
    }

    /// The inverse of [`render_list`](Self::render_list), with TOML's own
    /// error when the text is not a list of strings.
    pub fn parse_list(text: &str) -> Result<Vec<String>, String> {
        #[derive(Deserialize)]
        struct One {
            v: Vec<String>,
        }
        toml::from_str::<One>(&format!("v = {text}"))
            .map(|one| one.v)
            .map_err(|e| e.message().to_string())
    }
}

#[derive(Debug, Default, Deserialize)]
#[serde(deny_unknown_fields)]
struct RawClassify {
    #[serde(default)]
    generated: Vec<String>,
    #[serde(default)]
    not_generated: Vec<String>,
    #[serde(default)]
    attributes: Option<Vec<String>>,
}

#[derive(Debug)]
pub struct Config {
    /// Additive globs marking files as generated (noise-tier hint).
    pub generated: GlobSet,
    /// Overrides: never mark these generated. Wins over everything.
    pub not_generated: GlobSet,
    /// gitattributes attribute names honoured as "generated" declarations.
    /// Defaults to [`DEFAULT_ATTRIBUTES`]; setting the key **replaces** the
    /// list rather than adding to it.
    pub attributes: Vec<String>,
    /// From the USER config, never the repo (agents differ per user).
    pub grouping: GroupingConfig,
    /// From the USER config: how much context the reviewer shows.
    pub review: ReviewConfig,
    /// From the USER config: which keys the reviewer's actions answer to.
    pub keys: KeysConfig,
}

/// gitattributes names honoured as a "generated" declaration when
/// `[classify].attributes` is absent.
///
/// Two, because the convention is per-forge and a repository does not choose
/// its forge to suit this tool. `linguist-generated` is GitHub's, via Linguist;
/// `gitlab-generated` is GitLab's, and GitLab already honours it to collapse a
/// file in an MR diff — so a GitLab repository has usually declared its
/// generated files years before it meets this tool, and should not have to
/// declare them again.
///
/// The cost of an extra name is small and one-directional: a file has to carry
/// the attribute to match, and a repository that does not use a forge's
/// convention has nothing to match. A missed declaration is the expensive
/// direction — the file is offered to the model, grouped as real work, and read
/// by the reviewer.
pub const DEFAULT_ATTRIBUTES: &[&str] = &["linguist-generated", "gitlab-generated"];

fn default_attributes() -> Vec<String> {
    DEFAULT_ATTRIBUTES.iter().map(|s| s.to_string()).collect()
}

impl Default for Config {
    fn default() -> Self {
        Config {
            generated: GlobSet::empty(),
            not_generated: GlobSet::empty(),
            attributes: default_attributes(),
            grouping: GroupingConfig::default(),
            review: ReviewConfig::default(),
            keys: KeysConfig::default(),
        }
    }
}

/// `<user config dir>/differential/config.toml`.
///
/// The directory comes from `ConfigSource`; the two path components are
/// contract, not adapter, so they stay here.
pub fn user_config_path<S: crate::ports::ConfigSource>(src: &S) -> Option<PathBuf> {
    Some(
        src.user_config_dir()?
            .join(USER_CONFIG_DIR)
            .join(USER_CONFIG_FILE_NAME),
    )
}

impl Config {
    /// Resolution, per file: explicit path > default location > defaults.
    /// A missing file means defaults; a malformed file is a hard error, never
    /// silently ignored.
    ///
    /// Repo file: `<repo-root>/.differential.toml` — classification hints.
    /// User file: `~/.config/differential/config.toml` — `[grouping]`.
    pub fn load<S: crate::ports::ConfigSource>(
        src: &S,
        repo_root: &Path,
        repo_override: Option<&Path>,
        user_override: Option<&Path>,
    ) -> Result<Config, EngineError> {
        let repo_default = Some(repo_root.join(CONFIG_FILE_NAME));
        let mut config = match resolve(src, repo_override, repo_default)? {
            Some((text, origin)) => Self::parse(&text, &origin)?,
            None => Config::default(),
        };
        let user = Self::load_user(src, user_override)?;
        config.grouping = user.grouping;
        config.review = user.review;
        config.keys = user.keys;
        Ok(config)
    }

    /// The USER file alone: `[grouping]`, `[review]` and `[keys]`, and no
    /// repository.
    ///
    /// [`load`](Self::load) needs a repository root to find the repo file.
    /// `dfr agents` has none — which agent you would run is a per-user choice
    /// and the question is answerable from anywhere. Rather than hand it a
    /// directory it has no use for, the user half is its own call, and `load`
    /// goes through it so there is one answer to "where does the user file
    /// live".
    ///
    /// A missing file means defaults; a malformed one is a hard error.
    pub fn load_user<S: crate::ports::ConfigSource>(
        src: &S,
        user_override: Option<&Path>,
    ) -> Result<UserConfig, EngineError> {
        match resolve(src, user_override, user_config_path(src))? {
            Some((text, origin)) => Self::parse_user(&text, &origin),
            None => Ok(UserConfig::default()),
        }
    }

    /// Parse the REPO file: classification hints only. A `[grouping]` table
    /// here is a hard error with a pointer to its new home.
    pub fn parse(text: &str, origin: &str) -> Result<Config, EngineError> {
        let raw: RawConfig = toml::from_str(text).map_err(|e| EngineError::Config {
            path: origin.to_string(),
            msg: e.to_string(),
        })?;
        if raw.grouping.is_some() {
            return Err(EngineError::Config {
                path: origin.to_string(),
                msg: "[grouping] moved to the user config \
                      (~/.config/differential/config.toml): the agent command is a \
                      per-user choice, not a repo setting"
                    .to_string(),
            });
        }
        Ok(Config {
            generated: build_globs(&raw.classify.generated, origin)?,
            not_generated: build_globs(&raw.classify.not_generated, origin)?,
            attributes: raw.classify.attributes.unwrap_or_else(default_attributes),
            grouping: GroupingConfig::default(),
            review: ReviewConfig::default(),
            keys: KeysConfig::default(),
        })
    }

    /// Parse the USER file: `[grouping]`, `[review]` and `[keys]`.
    pub fn parse_user(text: &str, origin: &str) -> Result<UserConfig, EngineError> {
        let raw: RawUserConfig = toml::from_str(text).map_err(|e| EngineError::Config {
            path: origin.to_string(),
            msg: e.to_string(),
        })?;
        Ok(UserConfig {
            grouping: raw.grouping,
            review: raw.review,
            keys: raw.keys,
        })
    }
}

impl Config {
    /// The user file as TOML, whole. What [`save_user`](Self::save_user)
    /// writes: every `[review]` value, the `[grouping]` values that are set,
    /// and `[keys]` only when an action is rebound.
    pub fn render_user(user: &UserConfig) -> String {
        toml::to_string_pretty(user).expect("the user config is plain data and always serialises")
    }

    /// Write the user file at `path`, REPLACING it (ADR 0037).
    ///
    /// Whole-file on purpose: the reviewer's config modal edits every
    /// setting, and a rewrite is the one form that cannot disagree with what
    /// it shows. The cost is the file's comments and layout, which the modal
    /// says before it saves.
    ///
    /// The text is parsed back before it is written, so the file on disk is
    /// always one [`parse_user`](Self::parse_user) accepts, and accepts as
    /// this value.
    pub fn save_user<S: crate::ports::ConfigSource>(
        src: &S,
        path: &Path,
        user: &UserConfig,
    ) -> Result<(), EngineError> {
        let text = Self::render_user(user);
        let origin = path.display().to_string();
        let back = Self::parse_user(&text, &origin)?;
        if &back != user {
            return Err(EngineError::Config {
                path: origin,
                msg: "the config did not read back as it was written".into(),
            });
        }
        src.save(path, &text)
    }
}

/// Read (contents, origin) for `explicit > default`, where a missing default
/// is fine but a missing EXPLICIT path is a hard error.
///
/// The policy — which file, what precedence, what absence means — is here; the
/// port only hands back bytes. The two read methods exist so that an
/// explicit-but-missing path reports the same message it always did.
fn resolve<S: crate::ports::ConfigSource>(
    src: &S,
    explicit: Option<&Path>,
    default: Option<PathBuf>,
) -> Result<Option<(String, String)>, EngineError> {
    match explicit {
        Some(p) => Ok(Some((src.read_required(p)?, p.display().to_string()))),
        None => {
            let Some(p) = default else {
                return Ok(None);
            };
            Ok(src.read(&p)?.map(|text| (text, p.display().to_string())))
        }
    }
}

fn build_globs(patterns: &[String], origin: &str) -> Result<GlobSet, EngineError> {
    let mut b = GlobSetBuilder::new();
    for p in patterns {
        let glob = Glob::new(p).map_err(|e| EngineError::Config {
            path: origin.to_string(),
            msg: format!("bad glob {p:?}: {e}"),
        })?;
        b.add(glob);
    }
    b.build().map_err(|e| EngineError::Config {
        path: origin.to_string(),
        msg: e.to_string(),
    })
}

#[cfg(test)]
mod tests {
    /// The real filesystem: these assertions are about resolution policy
    /// (precedence, what absence means), which is what `load` owns.
    const SRC: crate::store::OsConfigSource = crate::store::OsConfigSource;

    use super::*;

    #[test]
    fn defaults_when_empty() {
        let c = Config::parse("", "test").unwrap();
        assert_eq!(c.attributes, ["linguist-generated", "gitlab-generated"]);
        // Both forge conventions out of the box: a repository does not choose
        // its forge to suit this tool, and a missed declaration is the
        // expensive direction — the file is offered to the model, grouped as
        // real work, and read.
        assert_eq!(c.attributes, DEFAULT_ATTRIBUTES);
        assert!(!c.generated.is_match("anything"));
    }

    #[test]
    fn globs_and_overrides() {
        let c = Config::parse(
            r#"
[classify]
generated = ["**/__snapshots__/**", "migrations/**"]
not_generated = ["important.lock"]
attributes = ["linguist-generated", "custom-generated"]
"#,
            "test",
        )
        .unwrap();
        assert!(c.generated.is_match("ui/__snapshots__/x.snap"));
        assert!(c.generated.is_match("migrations/0001_init.sql"));
        assert!(!c.generated.is_match("src/main.rs"));
        assert!(c.not_generated.is_match("important.lock"));
        // Setting the key REPLACES the default list; it does not extend it.
        // A repo naming only its own convention loses the forge ones, which is
        // the behaviour to know about rather than to discover.
        assert_eq!(c.attributes, ["linguist-generated", "custom-generated"]);
        let only_own =
            Config::parse("[classify]\nattributes = [\"custom-generated\"]", "test").unwrap();
        assert_eq!(only_own.attributes, ["custom-generated"]);
    }

    #[test]
    fn malformed_config_is_a_hard_error() {
        assert!(Config::parse("classify = 5", "test").is_err());
        assert!(Config::parse("[classify]\nnope = true", "test").is_err());
    }

    #[test]
    fn reserved_sections_are_accepted() {
        Config::parse("[ordering]\nfuture = 1\n[stack]\nns = \"y\"", "test").unwrap();
    }

    #[test]
    fn grouping_in_repo_config_errors_with_migration_hint() {
        let err = Config::parse("[grouping]\nagent = \"claude-code\"", "test").unwrap_err();
        assert!(err.to_string().contains("user config"), "{err}");
    }

    #[test]
    fn the_diff_layout_defaults_to_split_and_accepts_either_name() {
        // Absent means split. A reader who has never opened the config gets the
        // side-by-side layout.
        let u = Config::parse_user("[review]\ncontext = 3", "test").unwrap();
        assert_eq!(u.review.diff, DiffLayout::Split);
        assert!(u.review.diff.is_split());

        let u = Config::parse_user("[review]\ndiff = \"unified\"", "test").unwrap();
        assert_eq!(u.review.diff, DiffLayout::Unified);
        assert!(!u.review.diff.is_split());
        assert_eq!(u.review.context, 3, "setting one key must not zero another");

        let u = Config::parse_user("[review]\ndiff = \"split\"", "test").unwrap();
        assert_eq!(u.review.diff, DiffLayout::Split);

        // A typo is an error, not a silent fallback to the default.
        assert!(Config::parse_user("[review]\ndiff = \"side\"", "test").is_err());
    }

    #[test]
    fn user_config_parses_grouping_and_review() {
        let u = Config::parse_user(
            "[grouping]\nagent = \"claude-code\"\ntimeout_secs = 60",
            "test",
        )
        .unwrap();
        assert_eq!(u.grouping.agent, Some(Agent::ClaudeCode));
        assert_eq!(u.grouping.timeout_secs, Some(60));
        // An absent [review] means the defaults, not zero context.
        assert_eq!(u.review.context, 3);
        assert_eq!(u.review.context_step, 10);

        let u = Config::parse_user("[review]\ncontext_step = 25", "test").unwrap();
        assert_eq!(u.review.context_step, 25);
        assert_eq!(u.review.context, 3, "one key set must not zero the other");

        // Unknown keys and unknown sections stay hard errors.
        assert!(Config::parse_user("[grouping]\nmodel = \"x\"", "test").is_err());

        // An agent nobody implements is a hard error that names every one that
        // exists. A silent fall back to the default would run a different agent
        // than the one asked for, and the cache key would agree with neither.
        let err = Config::parse_user("[grouping]\nagent = \"gpt\"", "test").unwrap_err();
        let text = err.to_string();
        for &agent in Agent::ALL {
            assert!(
                text.contains(agent.key()),
                "the error must name {}: {text}",
                agent.key()
            );
        }

        // And the argv this key used to take is now one of those errors, not a
        // command that gets spawned without its allowlist.
        assert!(Config::parse_user("[grouping]\nagent = [\"my-llm\"]", "test").is_err());
        assert!(Config::parse_user("[review]\nlines = 5", "test").is_err());
        assert!(Config::parse_user("[classify]\ngenerated = []", "test").is_err());
    }

    #[test]
    fn every_agent_name_round_trips() {
        // `key` comes from strum and parsing from serde: two derives, each
        // with its own rename rule, so the two can drift. They may not: `key` is what the docs print, what
        // `dfr agents` lists and what an error message offers, and a name a
        // user copies from any of those must parse.
        for &agent in Agent::ALL {
            let toml = format!("[grouping]\nagent = \"{}\"", agent.key());
            let u = Config::parse_user(&toml, "test")
                .unwrap_or_else(|e| panic!("{} must parse: {e}", agent.key()));
            assert_eq!(u.grouping.agent, Some(agent), "{}", agent.key());
        }
    }

    #[test]
    fn which_agents_have_actually_been_run_is_pinned() {
        // `proven` is a claim about what a human ran, so nothing can check it
        // automatically. Pinning the two lists is the next best thing: moving
        // an agent between them has to be deliberate, and the reviewer of that
        // diff is being asked "did you run the probe?".
        let proven: Vec<&str> = Agent::ALL
            .iter()
            .filter(|a| a.proven())
            .map(|a| a.key())
            .collect();
        assert_eq!(proven, vec!["claude-code", "codex", "pi"]);

        let unproven: Vec<&str> = Agent::ALL
            .iter()
            .filter(|a| !a.proven())
            .map(|a| a.key())
            .collect();
        assert_eq!(unproven, vec!["droid", "copilot"]);

        // The default must be one somebody has run. Anything else ships a
        // command line nobody has tried to the people who configured nothing.
        assert!(Agent::default().proven(), "the default must be proven");
    }

    #[test]
    fn exactly_one_agent_does_not_enforce_read_only() {
        // The tier is a fact a user must be shown, so it is pinned here rather
        // than left to a doc comment. Pi ships no sandbox and no per-command
        // allowlist (ADR 0033); the other four refuse a write.
        let unenforced: Vec<&str> = Agent::ALL
            .iter()
            .filter(|a| !a.read_only().is_enforced())
            .map(|a| a.key())
            .collect();
        assert_eq!(unenforced, vec!["pi"], "{unenforced:?}");
        assert!(
            Agent::default().read_only().is_enforced(),
            "the default must be enforced"
        );
    }

    /// A theme is a per-user choice like the agent, named for the same reason:
    /// serde renders the valid names for free, and adding one is a variant.
    #[test]
    fn user_config_parses_the_theme_and_names_the_valid_ones() {
        let u = Config::parse_user("[review]\ntheme = \"gruvbox-light\"", "test").unwrap();
        assert_eq!(u.review.theme, ThemeName::GruvboxLight);
        // Absent is the default, and does not zero the other keys.
        let u = Config::parse_user("[review]\ncontext = 8", "test").unwrap();
        assert_eq!(u.review.theme, ThemeName::Dark);
        assert_eq!(u.review.context, 8);

        // An unknown name is an error that says which ones exist.
        let err = Config::parse_user("[review]\ntheme = \"nosferatu\"", "test").unwrap_err();
        let msg = err.to_string();
        for name in [
            "dark",
            "light",
            "gruvbox-dark",
            "solarized-light",
            "monokai",
        ] {
            assert!(msg.contains(name), "{name} missing from: {msg}");
        }
    }

    /// The editor is a COMMAND where `agent` and `theme` are names, so the
    /// thing to pin is that the command survives splitting and that the two
    /// placeholders land where the reader put them.
    #[test]
    fn the_editor_command_puts_the_path_and_the_line_where_the_reader_said() {
        let u = Config::parse_user("[review]\neditor = \"nvim +{line} {file}\"", "test").unwrap();
        let cmd = EditorCommand::parse(u.review.editor.as_deref().unwrap(), "test").unwrap();
        assert_eq!(
            cmd.argv(Path::new("/w/src/x.rs"), 42),
            ["nvim", "+42", "/w/src/x.rs"]
        );
        assert!(cmd.carries_line());
        assert_eq!(cmd.program(), "nvim");

        // Both placeholders in ONE word, which is how several editors spell it.
        // Splitting has to happen before substitution or this becomes three.
        let cmd = EditorCommand::parse("code -g {file}:{line}", "test").unwrap();
        assert_eq!(
            cmd.argv(Path::new("/w/src/x.rs"), 7),
            ["code", "-g", "/w/src/x.rs:7"]
        );

        // A quoted program with a space stays one word. This is the case a
        // hand-rolled splitter gets wrong, and why `shlex` does it.
        let cmd =
            EditorCommand::parse("\"/Applications/My Editor\" --at {line} {file}", "test").unwrap();
        assert_eq!(
            cmd.argv(Path::new("/w/x.rs"), 3),
            ["/Applications/My Editor", "--at", "3", "/w/x.rs"]
        );
    }

    /// A bare `$EDITOR` is the common case and names no placeholder at all.
    /// It must still open the file — at the top, and `carries_line` is what
    /// lets the caller say so rather than leave the reader wondering.
    #[test]
    fn a_command_naming_no_placeholder_still_gets_the_path() {
        let cmd = EditorCommand::parse("vim", "test").unwrap();
        assert_eq!(cmd.argv(Path::new("/w/x.rs"), 42), ["vim", "/w/x.rs"]);
        assert!(!cmd.carries_line());

        // Flags are kept, and the path still lands last.
        let cmd = EditorCommand::parse("emacsclient -nw", "test").unwrap();
        assert_eq!(
            cmd.argv(Path::new("/w/x.rs"), 42),
            ["emacsclient", "-nw", "/w/x.rs"]
        );
        assert!(!cmd.carries_line());

        // `{line}` without `{file}`: the line is honoured and the path is
        // still appended, so `+42` in front of it is the vim spelling.
        let cmd = EditorCommand::parse("vim +{line}", "test").unwrap();
        assert_eq!(
            cmd.argv(Path::new("/w/x.rs"), 42),
            ["vim", "+42", "/w/x.rs"]
        );
        assert!(cmd.carries_line());
    }

    /// Order is load-bearing: `{line}` goes first, so a path that happens to
    /// hold the text `{line}` is inserted and never read again.
    #[test]
    fn a_path_holding_a_placeholder_is_not_read_as_one() {
        let cmd = EditorCommand::parse("nvim +{line} {file}", "test").unwrap();
        assert_eq!(
            cmd.argv(Path::new("/w/{line}/x.rs"), 9),
            ["nvim", "+9", "/w/{line}/x.rs"]
        );
        // And the other way: a path holding `{file}` is not re-expanded
        // either, because `str::replace` does not re-scan what it inserts.
        assert_eq!(
            cmd.argv(Path::new("/w/{file}/x.rs"), 9),
            ["nvim", "+9", "/w/{file}/x.rs"]
        );
    }

    /// A command that cannot be run is an error at load, not a key that does
    /// nothing when it is pressed.
    #[test]
    fn an_unrunnable_editor_command_is_an_error() {
        let err = EditorCommand::parse("", "test").unwrap_err();
        assert!(err.to_string().contains("empty"), "{err}");
        // Whitespace alone splits to nothing, which is the same emptiness.
        assert!(EditorCommand::parse("   ", "test").is_err());
        // An unbalanced quote: `shlex` refuses, and so do we.
        let err = EditorCommand::parse("vim \"unclosed", "test").unwrap_err();
        assert!(err.to_string().contains("quote"), "{err}");
    }

    /// A placeholder in the FIRST word would make the program the file. A
    /// source file with the executable bit set would then be run by `e`, so
    /// this is refused at load rather than left to the operating system —
    /// which, for that file, would not refuse it at all.
    #[test]
    fn the_program_word_may_not_be_a_placeholder() {
        for bad in ["{file}", "{file} {line}", "{line}", "pre{file}post vim"] {
            let err = EditorCommand::parse(bad, "test").unwrap_err().to_string();
            assert!(err.contains("first word"), "{bad:?} gave {err}");
        }
        // A placeholder anywhere else is the whole point of the feature.
        assert!(EditorCommand::parse("vim +{line} {file}", "test").is_ok());
        assert!(EditorCommand::parse("code -g {file}:{line}", "test").is_ok());
    }

    /// The house rule for every key in this table: setting one must not zero
    /// another, and the key is absent by default.
    #[test]
    fn the_editor_key_is_optional_and_independent() {
        let u = Config::parse_user("[review]\ncontext = 8", "test").unwrap();
        assert_eq!(u.review.editor, None);

        let u = Config::parse_user("[review]\neditor = \"hx {file}:{line}\"", "test").unwrap();
        assert_eq!(u.review.editor.as_deref(), Some("hx {file}:{line}"));
        assert_eq!(u.review.context, 3);
        assert_eq!(u.review.context_step, 10);
        assert_eq!(u.review.theme, ThemeName::Dark);
        assert_eq!(u.review.diff, DiffLayout::Split);
    }

    /// An editor is the reader's, not the repository's — the same reason a
    /// palette is. Nothing enforces it by hand; the repo file has no
    /// `[review]` field and denies unknown ones.
    #[test]
    fn an_editor_in_the_repo_config_is_rejected() {
        assert!(Config::parse("[review]\neditor = \"vim\"", "test").is_err());
    }

    /// The repo file cannot set it: a palette is the reader's, not the
    /// repository's. Nothing enforces this by hand — `RawConfig` has no
    /// `[review]` and denies unknown fields.
    #[test]
    fn a_theme_in_the_repo_config_is_rejected() {
        let err = Config::parse("[review]\ntheme = \"one-light\"", "test").unwrap_err();
        assert!(err.to_string().contains("review"), "{err}");
    }

    #[test]
    fn load_composes_repo_and_user_files() {
        let tmp = tempfile::TempDir::new().unwrap();
        let repo_file = tmp.path().join("repo.toml");
        let user_file = tmp.path().join("user.toml");
        std::fs::write(&repo_file, "[classify]\ngenerated = [\"gen/**\"]").unwrap();
        std::fs::write(
            &user_file,
            "[grouping]\nagent = \"claude-code\"\n[review]\ncontext = 8\n[keys]\ntop = [\"Q\"]",
        )
        .unwrap();
        let c = Config::load(
            &crate::store::OsConfigSource,
            tmp.path(),
            Some(&repo_file),
            Some(&user_file),
        )
        .unwrap();
        assert!(c.generated.is_match("gen/x"));
        assert_eq!(c.grouping.agent, Some(Agent::ClaudeCode));
        assert_eq!(c.review.context, 8);
        assert_eq!(c.keys.0[&Action::Top], ["Q"]);

        // Explicit-but-missing paths are hard errors; absent defaults are not.
        assert!(
            Config::load(&SRC, tmp.path(), Some(Path::new("/nope")), Some(&user_file)).is_err()
        );
        assert!(Config::load(&SRC, tmp.path(), None, Some(&user_file)).is_ok());
    }

    #[test]
    fn keys_map_action_names_to_the_strings_as_written() {
        let u =
            Config::parse_user("[keys]\nnext-group = [\"ctrl-j\"]\npublish = []", "test").unwrap();
        assert_eq!(u.keys.0[&Action::NextGroup], ["ctrl-j"]);
        // An empty list is a statement, not an absence: it unbinds.
        assert_eq!(u.keys.0[&Action::Publish], Vec::<String>::new());
        assert!(
            !u.keys.0.contains_key(&Action::Top),
            "unnamed keeps defaults"
        );
        // Absent means no overrides at all.
        assert!(Config::parse_user("", "test").unwrap().keys.0.is_empty());
    }

    #[test]
    fn an_unknown_action_is_an_error_naming_every_action() {
        let err = Config::parse_user("[keys]\nexplode = [\"x\"]", "test").unwrap_err();
        let text = err.to_string();
        for &action in Action::ALL {
            assert!(
                text.contains(action.key()),
                "must name {}: {text}",
                action.key()
            );
        }
        // A bare string where a list goes is an error too, not a one-key list:
        // the shape is the same for one key as for three.
        assert!(Config::parse_user("[keys]\ntop = \"g\"", "test").is_err());
    }

    #[test]
    fn keys_are_the_users_and_not_the_repos() {
        let err = Config::parse("[keys]\ntop = [\"g\"]", "test").unwrap_err();
        assert!(err.to_string().contains("keys"), "{err}");
    }

    #[test]
    fn every_action_name_round_trips() {
        for &action in Action::ALL {
            let text = format!("[keys]\n{} = []", action.key());
            let u = Config::parse_user(&text, "test").unwrap();
            assert!(
                u.keys.0.contains_key(&action),
                "{} did not parse",
                action.key()
            );
        }
    }

    #[test]
    fn a_rendered_user_config_reads_back_as_itself() {
        let full = Config::parse_user(
            "[grouping]\nagent = \"codex\"\ntimeout_secs = 60\n\
             [review]\ntheme = \"gruvbox-light\"\ncontext = 8\ncontext_step = 4\ndiff = \"unified\"\n\
             [keys]\nnext-group = [\"ctrl-j\"]\npublish = []",
            "test",
        )
        .unwrap();
        for user in [full, UserConfig::default()] {
            let text = Config::render_user(&user);
            assert_eq!(Config::parse_user(&text, "test").unwrap(), user, "{text}");
        }
        // Nothing rebound, nothing about agents: no table for either.
        let text = Config::render_user(&UserConfig::default());
        assert!(
            !text.contains("[keys]") && !text.contains("agent"),
            "{text}"
        );
    }

    #[test]
    fn save_user_writes_the_file_and_its_directory() {
        let tmp = tempfile::TempDir::new().unwrap();
        let path = tmp.path().join("differential").join("config.toml");
        let mut user = UserConfig::default();
        user.review.theme = ThemeName::Dracula;
        Config::save_user(&SRC, &path, &user).unwrap();
        let back = Config::load_user(&SRC, Some(&path)).unwrap();
        assert_eq!(back, user);
    }

    #[test]
    fn every_theme_and_layout_name_round_trips() {
        for &theme in ThemeName::ALL {
            // The `match` is the exhaustiveness guard `ALL` cannot be.
            match theme {
                ThemeName::Dark
                | ThemeName::OneDark
                | ThemeName::OneLight
                | ThemeName::GruvboxDark
                | ThemeName::GruvboxLight
                | ThemeName::SolarizedDark
                | ThemeName::SolarizedLight
                | ThemeName::CatppuccinMocha
                | ThemeName::CatppuccinLatte
                | ThemeName::Dracula
                | ThemeName::Monokai => {}
            }
            let text = format!("[review]\ntheme = \"{}\"", theme.key());
            assert_eq!(
                Config::parse_user(&text, "test").unwrap().review.theme,
                theme
            );
        }
        for &diff in DiffLayout::ALL {
            match diff {
                DiffLayout::Split | DiffLayout::Unified => {}
            }
            let text = format!("[review]\ndiff = \"{}\"", diff.key());
            assert_eq!(Config::parse_user(&text, "test").unwrap().review.diff, diff);
        }
    }

    #[test]
    fn a_key_list_round_trips_in_the_files_syntax() {
        let keys = vec!["ctrl-j".to_string(), "d d".to_string(), "\"".to_string()];
        let text = KeysConfig::render_list(&keys);
        assert_eq!(KeysConfig::parse_list(&text).unwrap(), keys, "{text}");
        assert_eq!(KeysConfig::parse_list("[]").unwrap(), Vec::<String>::new());
        assert!(KeysConfig::parse_list("ctrl-j").is_err());
        assert!(KeysConfig::parse_list("[1]").is_err());
    }
}