mecha10_job_validation/

lib.rs

//! Shared validation for `mecha10 train`'s job catalog (`configs/rl/jobs/*.yml`) and its
//! `task_name` references into the task catalog (`configs/rl/tasks/*/task.json`).
//!
//! This crate is the single source of truth for two separate questions, deliberately kept in
//! two separate tables rather than one, the same way node configs separate "fields every node
//! has" ([`crate::...`] - see `packages/core/src/node_config_base.rs` in the main workspace,
//! `docs/adr/0001-node-config-base-schema.md`) from "fields specific to one node type":
//!
//! - "what's required/optional/not-applicable per `flow_type`" ([`JobField`]/[`validate_job`]) -
//!   algorithm-agnostic, flow-shape concepts like `task_name`/`resume_run_id`/`tune_run_id`.
//! - "what's required/optional/not-applicable per `algorithm`"
//!   ([`AlgorithmField`]/[`validate_algorithm_fields`]) - per-algorithm hyperparameter concepts
//!   like `total_timesteps`/`n_steps`, which vary mostly by which SB3 algorithm a job names
//!   rather than which flow it runs (with one carve-out: `ppo`'s `n_steps` is relaxed for a
//!   `resume-run`, which restores it from the checkpoint). See that section's own doc comment
//!   for the full rationale.
//!
//! Before this crate existed, both questions were answered ad hoc, in multiple places (the
//! CLI's job struct defaults, the CLI's dispatch-time `if` checks, and mecha10-api's
//! `create_job` handler), which could and did drift out of sync with each other - and even
//! after this crate existed, the per-algorithm question kept being answered with one-off
//! `if algorithm == "ppo"` checks in the CLI rather than a table here, which is exactly the
//! shape that silently broke when `act` was added (it has no `total_timesteps` field at all).
//! The CLI (`packages/cli`) depends on this crate and calls
//! [`validate_job`]/[`validate_algorithm_fields`]/[`task_exists`] instead of re-deriving these
//! rules locally; `mecha10-api`'s `create_job` handler (`services/api`) is intended to do the
//! same, but as of now does not depend on this crate and does not call any of them.
//!
//! Deliberately Python-free: `rl_task_runner.run` (the Python runtime) does not depend on this
//! crate and does not re-validate - it trusts that a job it's asked to run already passed
//! validation upstream (CLI local dispatch, or mecha10-api's `create_job` for remote dispatch).

use std::path::Path;

/// Which training pipeline flow a job runs as.
#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Deserialize, serde::Serialize, schemars::JsonSchema)]
#[serde(rename_all = "kebab-case")]
pub enum FlowType {
    Train,
    Tune,
    TuneThenTrain,
    TrainFromTune,
    ResumeRun,
}

impl std::fmt::Display for FlowType {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        let s = match self {
            FlowType::Train => "train",
            FlowType::Tune => "tune",
            FlowType::TuneThenTrain => "tune-then-train",
            FlowType::TrainFromTune => "train-from-tune",
            FlowType::ResumeRun => "resume-run",
        };
        write!(f, "{s}")
    }
}

/// Whether a field is required, optional (sentinel/default-backed), or not applicable for a
/// given `flow_type` - e.g. `resume_run_id` is `Required` for `ResumeRun`, `NotApplicable` for
/// `Train`.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum FieldRequirement {
    Required,
    Optional,
    NotApplicable,
}

/// The fields this crate has an opinion about. Deliberately not every `PPOConfig` hyperparameter
/// (`learning_rate`, `gamma`, `clip_range`, ...) - those are legitimately optional/sentinel-
/// defaulted for every `flow_type` today and aren't in dispute; this covers job identity
/// (`task_name`/`algorithm`) and the fields that control how long a flow runs or which prior run
/// it targets, which is where "required vs. optional vs. n/a" actually varies by `flow_type`.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum JobField {
    TaskName,
    Algorithm,
    ResumeRunId,
    TuneRunId,
    NTrials,
    TuneTimesteps,
    EvalEps,
}

impl JobField {
    /// The wire/YAML field name a `CreateJobRequest`/job-definition struct is expected to carry
    /// this field under - the single list a contract test in each consumer (the CLI's own
    /// `CreateJobRequest`, `mecha10-api`'s `CreateJobRequest`) checks its own hand-typed struct
    /// against, so a new variant here shows up as a failing test in both instead of silently
    /// missing a field the way `Tune`'s `n_trials`/`tune_timesteps`/`eval_eps` did before either
    /// request struct picked them up.
    pub fn wire_name(self) -> &'static str {
        match self {
            JobField::TaskName => "task_name",
            JobField::Algorithm => "algorithm",
            JobField::ResumeRunId => "resume_run_id",
            JobField::TuneRunId => "tune_run_id",
            JobField::NTrials => "n_trials",
            JobField::TuneTimesteps => "tune_timesteps",
            JobField::EvalEps => "eval_eps",
        }
    }

    /// Requiredness of this field for `flow_type`, the single table the CLI reads instead of
    /// re-deriving it; `mecha10-api` is intended to read it too, once it's wired up to depend on
    /// this crate.
    pub fn requirement(self, flow_type: FlowType) -> FieldRequirement {
        use FieldRequirement::*;
        use FlowType::*;
        use JobField::*;

        match (self, flow_type) {
            (TaskName, _) => Required,
            (Algorithm, _) => Required,

            (ResumeRunId, ResumeRun) => Required,
            (ResumeRunId, _) => NotApplicable,

            (TuneRunId, TrainFromTune) => Required,
            (TuneRunId, _) => NotApplicable,

            (NTrials | TuneTimesteps | EvalEps, Tune | TuneThenTrain) => Required,
            (NTrials | TuneTimesteps | EvalEps, _) => NotApplicable,
        }
    }
}

/// The subset of a job definition's fields this crate validates. The CLI's `JobDefinition`
/// (`packages/cli/src/services/train_jobs.rs`) and mecha10-api's `CreateJobRequest`
/// (`services/api/src/jobs/types.rs`) each convert their own richer type into this one (via a
/// small `From`/helper on their side) rather than this crate depending on either of them -
/// this stays the shared validation vocabulary, not the wire/storage shape.
#[derive(Debug, Clone, Default)]
pub struct JobFields {
    pub task_name: Option<String>,
    pub algorithm: Option<String>,
    pub resume_run_id: Option<String>,
    pub tune_run_id: Option<String>,
    pub n_trials: Option<i64>,
    pub tune_timesteps: Option<i64>,
    pub eval_eps: Option<i64>,
}

impl JobFields {
    fn get(&self, field: JobField) -> FieldPresence<'_> {
        // None of the base job fields have a `patience`/`min_delta`-style meaningful-sentinel
        // convention today - a `-1` on `n_trials`/`tune_timesteps`/`eval_eps` is always just a
        // "not set" placeholder, never a deliberate value - so every int field here uses
        // `NotSetPlaceholder`.
        match field {
            JobField::TaskName => FieldPresence::Str(self.task_name.as_deref()),
            JobField::Algorithm => FieldPresence::Str(self.algorithm.as_deref()),
            JobField::ResumeRunId => FieldPresence::Str(self.resume_run_id.as_deref()),
            JobField::TuneRunId => FieldPresence::Str(self.tune_run_id.as_deref()),
            JobField::NTrials => FieldPresence::Int(self.n_trials, SentinelConvention::NotSetPlaceholder),
            JobField::TuneTimesteps => FieldPresence::Int(self.tune_timesteps, SentinelConvention::NotSetPlaceholder),
            JobField::EvalEps => FieldPresence::Int(self.eval_eps, SentinelConvention::NotSetPlaceholder),
        }
    }
}

/// Whether a numeric field's `-1`/`-1.0` sentinel is a genuine, intended value (e.g. ppo's
/// `patience`/`min_delta`, where `-1` means "no early-stopping limit" - a real hyperparameter
/// choice the training run honors) or just a placeholder some job-authoring path uses to mean
/// "this was never actually set" (every other numeric field this crate validates today, e.g.
/// `total_timesteps`/`n_steps` - a job left at `-1` there is missing a real value, not expressing
/// a deliberate choice).
///
/// This is what used to be answered outside this crate, ad hoc, by having every *caller* run
/// every numeric field through the same blanket "drop `-1`/`-1.0`" helper
/// (`non_sentinel_i64`/`non_sentinel_f64` in `packages/cli/src/services/train_jobs.rs`) before
/// validation ever saw the value - which is exactly how a real `patience: -1`/`min_delta: -1.0`
/// (the scaffolded `train.yml` template's own values, see `mecha10-templates`'
/// `rl_bundle.rs::FIXTURE_TRAIN`) got reported as "missing" instead of "set to a value that means
/// disabled": the blanket helper can't tell `patience`'s `-1` (meaningful) apart from
/// `total_timesteps`'s `-1` (placeholder) because it never knew which field it was stripping.
/// Making that distinction part of each field's own schema entry here - not a strip applied
/// before validation runs - is what lets validation tell them apart. See PR #1639 (attempted this
/// as a pre-validation strip, special-cased to just `patience`/`min_delta`) and its revert PR
/// #1641 (determined this needed real schema design, not a quick patch).
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum SentinelConvention {
    /// `-1`/`-1.0` is a legitimate, meaningful value for this field - presence is simply whether
    /// the field was set at all (`Option::is_some()`); the crate does not second-guess the value
    /// itself.
    MeaningfulValue,
    /// `-1`/`-1.0` has no meaning for this field; it's only ever a "not set" placeholder, so a
    /// field left at that value counts as absent for requiredness purposes, same as `None`.
    NotSetPlaceholder,
}

/// The `-1` placeholder [`SentinelConvention::NotSetPlaceholder`] int fields use for "not set".
const SENTINEL_I64: i64 = -1;
/// The `-1.0` placeholder [`SentinelConvention::NotSetPlaceholder`] float fields use for "not
/// set".
const SENTINEL_F64: f64 = -1.0;

enum FieldPresence<'a> {
    Str(Option<&'a str>),
    Int(Option<i64>, SentinelConvention),
    Float(Option<f64>, SentinelConvention),
}

impl FieldPresence<'_> {
    /// A string field counts as absent when blank/whitespace-only (matches the CLI's existing
    /// `.trim().is_empty()` checks for `algorithm`/`resume_run_id`). An int/float field counts as
    /// absent when unset, or - for a [`SentinelConvention::NotSetPlaceholder`] field only - when
    /// set to that field's `-1`/`-1.0` placeholder; a [`SentinelConvention::MeaningfulValue`]
    /// field (e.g. ppo's `patience`/`min_delta`) counts as present at `-1`/`-1.0` too, since that
    /// value is a deliberate choice, not an unset marker.
    fn is_present(&self) -> bool {
        match self {
            FieldPresence::Str(s) => s.is_some_and(|s| !s.trim().is_empty()),
            FieldPresence::Int(i, convention) => match convention {
                SentinelConvention::MeaningfulValue => i.is_some(),
                SentinelConvention::NotSetPlaceholder => i.is_some_and(|v| v != SENTINEL_I64),
            },
            FieldPresence::Float(f, convention) => match convention {
                SentinelConvention::MeaningfulValue => f.is_some(),
                SentinelConvention::NotSetPlaceholder => f.is_some_and(|v| v != SENTINEL_F64),
            },
        }
    }
}

/// One validation failure: a field that's `Required` for this job's `flow_type` but absent.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct MissingField {
    pub field: JobField,
    pub flow_type: FlowType,
}

impl std::fmt::Display for MissingField {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(
            f,
            "'{:?}' is required for flow_type '{}' but was not set",
            self.field, self.flow_type
        )
    }
}

const ALL_FIELDS: &[JobField] = &[
    JobField::TaskName,
    JobField::Algorithm,
    JobField::ResumeRunId,
    JobField::TuneRunId,
    JobField::NTrials,
    JobField::TuneTimesteps,
    JobField::EvalEps,
];

/// Every [`JobField`] this crate has an opinion about - the list a consumer's own contract
/// test (asserting its hand-typed `CreateJobRequest` carries a matching field for each one
/// that's applicable to a flow type it actually dispatches) iterates over.
pub fn all_fields() -> &'static [JobField] {
    ALL_FIELDS
}

/// Validate `fields` against `flow_type`'s requirement table. Returns every missing required
/// field (not just the first), so a caller can report the whole list at once.
pub fn validate_job(flow_type: FlowType, fields: &JobFields) -> Vec<MissingField> {
    ALL_FIELDS
        .iter()
        .filter(|&&field| field.requirement(flow_type) == FieldRequirement::Required)
        .filter(|&&field| !fields.get(field).is_present())
        .map(|&field| MissingField { field, flow_type })
        .collect()
}

// --- By-algorithm schema ---------------------------------------------------------------------
//
// [`JobField`]/[`validate_job`] above answer "what's required for this job's `flow_type`" - the
// algorithm-agnostic, flow-shape questions. They deliberately don't answer "what does this
// *algorithm* need" - `total_timesteps`/`learning_rate`/`n_steps`/`n_epochs`/`patience`/
// `min_delta` aren't flow_type concepts, they're per-algorithm concepts: each SB3 algorithm's
// own Python config dataclass in the separate `mecha10-rl` repo
// (`rl-task-runner/rl_task_runner/algorithms/<name>.py`) declares its own subset of these with
// no default, so a job that omits one for the algorithm it names crashes training at runtime
// with a plain `TypeError` instead of failing validation up front. This table is this crate's
// "custom schema" layer - the same idea as `NodeConfigBase`/`compose_node_schema`'s base/custom
// split for node configs (`docs/adr/0001-node-config-base-schema.md`), except keyed by
// algorithm name instead of node type, and composed generically by the caller (see
// `JobDefinition::validate` in `packages/cli/src/services/train_jobs.rs`) instead of via a
// one-off `if algorithm == "ppo"` check that risks silently missing the next new algorithm the
// way `act` did.

/// The per-algorithm hyperparameter fields this crate has an opinion about. See the module-level
/// comment above for why these (and not every `PPOConfig` field) are the ones in dispute.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum AlgorithmField {
    TotalTimesteps,
    LearningRate,
    NSteps,
    NEpochs,
    Patience,
    MinDelta,
}

impl AlgorithmField {
    /// The wire/YAML field name this field is expected to carry - mirrors
    /// [`JobField::wire_name`].
    pub fn wire_name(self) -> &'static str {
        match self {
            AlgorithmField::TotalTimesteps => "total_timesteps",
            AlgorithmField::LearningRate => "learning_rate",
            AlgorithmField::NSteps => "n_steps",
            AlgorithmField::NEpochs => "n_epochs",
            AlgorithmField::Patience => "patience",
            AlgorithmField::MinDelta => "min_delta",
        }
    }

    /// Requiredness of this field for `algorithm` on `flow_type` - the single table a caller
    /// reads instead of re-deriving it with its own `if algorithm == "..."` checks.
    ///
    /// Requiredness is keyed primarily by algorithm (`total_timesteps`/`learning_rate`/
    /// `n_steps`/`n_epochs`/`patience`/`min_delta` are per-algorithm hyperparameter concepts,
    /// not flow-shape ones), with one flow_type-specific carve-out: `n_steps` is `Optional`, not
    /// `Required`, for a `ppo` `resume-run` job. A resume-run restores the rollout length from
    /// the checkpoint it resumes, so that one field need not be re-specified - this preserves the
    /// pre-existing hand-rolled behavior (`flow_type != ResumeRun && algorithm == "ppo" &&
    /// n_steps missing`) that a one-off `if` used to encode in the CLI. Every *other* `ppo`
    /// hyperparameter (`total_timesteps`/`learning_rate`/`n_epochs`/`patience`/`min_delta`)
    /// stays `Required` even for a `ppo` resume-run - only `n_steps` had this carve-out.
    ///
    /// `ppo` is the only algorithm with an on-policy rollout (`n_steps`) and a separate PPO
    /// epoch count (`n_epochs`), so those two are `NotApplicable` for every other algorithm.
    /// `sac`/`td3`/`a2c` still require the remaining four (`a2c.py`'s `n_steps` has a default of
    /// `5`, so it's not in dispute there either - only the fields with *no* Python default
    /// belong in this table). `act` (imitation learning, no SB3 on-policy/off-policy config at
    /// all) and any unrecognized/future algorithm name (including an unset/`None` algorithm,
    /// which callers pass as `""`) resolve every field to `NotApplicable`, so an algorithm this
    /// table doesn't know about yet fails open - no spurious "missing field" errors - rather
    /// than silently inheriting `ppo`'s requirements.
    ///
    /// If `act`'s own no-default field (`dataset_path`) ever becomes a typed, validated
    /// `JobDefinition` field, it gets its own `AlgorithmField` variant here, not another
    /// one-off `if`.
    pub fn requirement(self, algorithm: &str, flow_type: FlowType) -> FieldRequirement {
        use AlgorithmField::*;
        use FieldRequirement::*;

        // A `ppo` resume-run reuses the rollout length (`n_steps`) baked into the checkpoint it
        // resumes, so `n_steps` specifically is not required there - but every other `ppo`
        // hyperparameter still is (handled by the `match` below).
        if self == NSteps && algorithm == "ppo" && flow_type == FlowType::ResumeRun {
            return Optional;
        }

        match algorithm {
            "ppo" => Required,
            "sac" | "td3" | "a2c" => match self {
                TotalTimesteps | LearningRate | Patience | MinDelta => Required,
                NSteps | NEpochs => NotApplicable,
            },
            _ => NotApplicable,
        }
    }

    /// This field's [`SentinelConvention`] - whether `-1`/`-1.0` is a real, intended value for
    /// this field or just a "not set" placeholder. See [`SentinelConvention`]'s doc comment for
    /// the bug this distinction exists to fix.
    ///
    /// Only `patience`/`min_delta` carry a deliberate "disabled" meaning at `-1`/`-1.0` - that's
    /// an early-stopping concept every algorithm that has these fields (`ppo`/`sac`/`td3`/`a2c`,
    /// per [`Self::requirement`]) shares, so this is a per-field distinction rather than a
    /// per-(field, algorithm) one today. If a future algorithm needs a different convention for
    /// one of these fields, this is where that split would go - taking an `algorithm: &str`
    /// parameter the same way [`Self::requirement`] does.
    pub fn sentinel_convention(self) -> SentinelConvention {
        match self {
            AlgorithmField::Patience | AlgorithmField::MinDelta => SentinelConvention::MeaningfulValue,
            AlgorithmField::TotalTimesteps
            | AlgorithmField::LearningRate
            | AlgorithmField::NSteps
            | AlgorithmField::NEpochs => SentinelConvention::NotSetPlaceholder,
        }
    }
}

/// The subset of a job definition's per-algorithm hyperparameter fields this crate validates -
/// the by-algorithm counterpart to [`JobFields`]. See [`AlgorithmField`]'s doc comment for why
/// these particular fields.
#[derive(Debug, Clone, Default)]
pub struct AlgorithmFields {
    pub total_timesteps: Option<i64>,
    pub learning_rate: Option<f64>,
    pub n_steps: Option<i64>,
    pub n_epochs: Option<i64>,
    pub patience: Option<i64>,
    pub min_delta: Option<f64>,
}

impl AlgorithmFields {
    /// Builds this field's [`FieldPresence`] directly from the raw value this struct was given -
    /// callers (the CLI's `JobDefinition::validation_fields_for_algorithm`) pass the job's value
    /// through untouched, no pre-stripping. [`AlgorithmField::sentinel_convention`] is what tells
    /// [`FieldPresence::is_present`] whether a `-1`/`-1.0` on this particular field counts as set.
    fn get(&self, field: AlgorithmField) -> FieldPresence<'_> {
        let convention = field.sentinel_convention();
        match field {
            AlgorithmField::TotalTimesteps => FieldPresence::Int(self.total_timesteps, convention),
            AlgorithmField::LearningRate => FieldPresence::Float(self.learning_rate, convention),
            AlgorithmField::NSteps => FieldPresence::Int(self.n_steps, convention),
            AlgorithmField::NEpochs => FieldPresence::Int(self.n_epochs, convention),
            AlgorithmField::Patience => FieldPresence::Int(self.patience, convention),
            AlgorithmField::MinDelta => FieldPresence::Float(self.min_delta, convention),
        }
    }
}

/// One by-algorithm validation failure: a field that's `Required` for `algorithm` but absent -
/// the by-algorithm counterpart to [`MissingField`].
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct MissingAlgorithmField {
    pub field: AlgorithmField,
    pub algorithm: String,
}

impl std::fmt::Display for MissingAlgorithmField {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(
            f,
            "'{}' is required for algorithm '{}' but was not set",
            self.field.wire_name(),
            self.algorithm
        )
    }
}

const ALL_ALGORITHM_FIELDS: &[AlgorithmField] = &[
    AlgorithmField::TotalTimesteps,
    AlgorithmField::LearningRate,
    AlgorithmField::NSteps,
    AlgorithmField::NEpochs,
    AlgorithmField::Patience,
    AlgorithmField::MinDelta,
];

/// Every [`AlgorithmField`] this crate has an opinion about - the by-algorithm counterpart to
/// [`all_fields`].
pub fn all_algorithm_fields() -> &'static [AlgorithmField] {
    ALL_ALGORITHM_FIELDS
}

/// Validate `fields` against `algorithm`'s requirement table for `flow_type`. Returns every
/// missing required field (not just the first), so a caller can report the whole list at once -
/// the by-algorithm counterpart to [`validate_job`]. `flow_type` is needed because a few
/// per-algorithm requirements are relaxed for specific flows (e.g. `ppo`'s `n_steps` is not
/// required for a `resume-run`, which restores it from the checkpoint) - see
/// [`AlgorithmField::requirement`].
pub fn validate_algorithm_fields(
    algorithm: &str,
    flow_type: FlowType,
    fields: &AlgorithmFields,
) -> Vec<MissingAlgorithmField> {
    ALL_ALGORITHM_FIELDS
        .iter()
        .filter(|&&field| field.requirement(algorithm, flow_type) == FieldRequirement::Required)
        .filter(|&&field| !fields.get(field).is_present())
        .map(|&field| MissingAlgorithmField {
            field,
            algorithm: algorithm.to_string(),
        })
        .collect()
}

/// Error from [`task_exists`]: `task_name` has no corresponding `task.json` under the task
/// catalog, or that file isn't valid JSON.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum TaskLookupError {
    NotFound { task_name: String },
    InvalidJson { task_name: String, message: String },
}

impl std::fmt::Display for TaskLookupError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            TaskLookupError::NotFound { task_name } => write!(
                f,
                "task '{task_name}' has no configs/rl/tasks/{task_name}/task.json - check the \
                 task_name against the project's task catalog"
            ),
            TaskLookupError::InvalidJson { task_name, message } => {
                write!(f, "configs/rl/tasks/{task_name}/task.json is not valid JSON: {message}")
            }
        }
    }
}

impl std::error::Error for TaskLookupError {}

/// `configs/rl/tasks` relative to a project root - kept as a single literal here (rather than a
/// dependency on `mecha10-cli-core`, to keep this crate's dependency footprint minimal for
/// mecha10-api's sake) and covered by `RL_TASKS_DIR`'s own equality test in
/// `mecha10-cli-core::paths::project`, so the two can't silently drift.
pub const RL_TASKS_DIR: &str = "configs/rl/tasks";

/// Confirm `task_name` has a real `task.json` under `{project_root}/configs/rl/tasks/` and that
/// it parses as JSON. This is the check that was previously only ever done, best-effort, deep in
/// `rl_task_runner.run` at actual training time (and even there, on failure it only logged a
/// warning and continued rather than failing) - moving it here means a bad `task_name` is caught
/// at CLI/API accept-time instead of surfacing as unexplained training behavior much later.
pub fn task_exists(project_root: &Path, task_name: &str) -> Result<(), TaskLookupError> {
    let path = project_root.join(RL_TASKS_DIR).join(task_name).join("task.json");
    let content = std::fs::read_to_string(&path).map_err(|_| TaskLookupError::NotFound {
        task_name: task_name.to_string(),
    })?;
    serde_json::from_str::<serde_json::Value>(&content).map_err(|e| TaskLookupError::InvalidJson {
        task_name: task_name.to_string(),
        message: e.to_string(),
    })?;
    Ok(())
}

/// List every task name under `{project_root}/configs/rl/tasks/` that has a `task.json` - the
/// catalog `mecha10 train`'s task picker prompts from after a job is selected. A job's own
/// `configs/rl/jobs/*.yml` no longer pins `task_name` itself (that field is filled in at
/// dispatch time from this picker instead); this is the list it prompts against. Sorted for a
/// stable menu order; empty (not an error) when the directory doesn't exist.
pub fn list_tasks(project_root: &Path) -> std::io::Result<Vec<String>> {
    let dir = project_root.join(RL_TASKS_DIR);
    if !dir.exists() {
        return Ok(Vec::new());
    }

    let mut names: Vec<String> = std::fs::read_dir(&dir)?
        .filter_map(|entry| entry.ok())
        .filter(|entry| entry.path().join("task.json").is_file())
        .filter_map(|entry| entry.file_name().into_string().ok())
        .collect();
    names.sort();
    Ok(names)
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn every_job_field_has_a_unique_wire_name() {
        let names: Vec<&str> = ALL_FIELDS.iter().map(|f| f.wire_name()).collect();
        let unique: std::collections::HashSet<&str> = names.iter().copied().collect();
        assert_eq!(names.len(), unique.len());
    }

    fn fields_with_task_and_algo() -> JobFields {
        JobFields {
            task_name: Some("find_target".to_string()),
            algorithm: Some("ppo".to_string()),
            ..Default::default()
        }
    }

    #[test]
    fn train_requires_only_task_name_and_algorithm() {
        // `total_timesteps` used to be required here too, but it's a per-algorithm concept
        // (not every flow_type), so it's validated by `validate_algorithm_fields` now instead -
        // see the `algorithm_field_tests` module below.
        let fields = fields_with_task_and_algo();
        assert!(validate_job(FlowType::Train, &fields).is_empty());
    }

    #[test]
    fn tune_requires_tune_specific_fields() {
        let fields = fields_with_task_and_algo();
        let missing = validate_job(FlowType::Tune, &fields);
        assert_eq!(missing.len(), 3);
        assert!(missing.iter().all(|m| m.flow_type == FlowType::Tune));
    }

    #[test]
    fn tune_passes_with_tune_specific_fields_set() {
        let mut fields = fields_with_task_and_algo();
        fields.n_trials = Some(10);
        fields.tune_timesteps = Some(1000);
        fields.eval_eps = Some(2);
        assert!(validate_job(FlowType::Tune, &fields).is_empty());
    }

    #[test]
    fn resume_run_requires_resume_run_id() {
        let fields = fields_with_task_and_algo();
        let missing = validate_job(FlowType::ResumeRun, &fields);
        assert_eq!(
            missing,
            vec![MissingField {
                field: JobField::ResumeRunId,
                flow_type: FlowType::ResumeRun
            }]
        );
    }

    #[test]
    fn train_from_tune_requires_tune_run_id() {
        let fields = fields_with_task_and_algo();
        let missing = validate_job(FlowType::TrainFromTune, &fields);
        assert_eq!(
            missing,
            vec![MissingField {
                field: JobField::TuneRunId,
                flow_type: FlowType::TrainFromTune
            }]
        );
    }

    #[test]
    fn missing_task_name_and_algorithm_reported_for_every_flow_type() {
        let missing = validate_job(FlowType::Train, &JobFields::default());
        assert!(missing.iter().any(|m| m.field == JobField::TaskName));
        assert!(missing.iter().any(|m| m.field == JobField::Algorithm));
    }

    #[test]
    fn blank_string_fields_count_as_absent() {
        let mut fields = fields_with_task_and_algo();
        fields.resume_run_id = Some("   ".to_string());
        let missing = validate_job(FlowType::ResumeRun, &fields);
        assert!(missing.iter().any(|m| m.field == JobField::ResumeRunId));
    }

    #[test]
    fn task_exists_finds_bundled_find_target_task() {
        let dir = tempfile::tempdir().unwrap();
        let task_dir = dir.path().join("configs/rl/tasks/find_target");
        std::fs::create_dir_all(&task_dir).unwrap();
        std::fs::write(task_dir.join("task.json"), r#"{"env_name": "basic_arena"}"#).unwrap();
        assert!(task_exists(dir.path(), "find_target").is_ok());
    }

    #[test]
    fn task_exists_reports_missing_task() {
        let dir = tempfile::tempdir().unwrap();
        std::fs::create_dir_all(dir.path().join("configs/rl/tasks")).unwrap();
        let err = task_exists(dir.path(), "nonexistent").unwrap_err();
        assert_eq!(
            err,
            TaskLookupError::NotFound {
                task_name: "nonexistent".to_string()
            }
        );
    }

    #[test]
    fn list_tasks_returns_empty_when_directory_missing() {
        let dir = tempfile::tempdir().unwrap();
        assert_eq!(list_tasks(dir.path()).unwrap(), Vec::<String>::new());
    }

    #[test]
    fn list_tasks_lists_directories_with_a_task_json_sorted() {
        let dir = tempfile::tempdir().unwrap();
        for name in ["task2", "find_target", "task1"] {
            let task_dir = dir.path().join("configs/rl/tasks").join(name);
            std::fs::create_dir_all(&task_dir).unwrap();
            std::fs::write(task_dir.join("task.json"), "{}").unwrap();
        }
        std::fs::create_dir_all(dir.path().join("configs/rl/tasks/not-a-task")).unwrap();

        assert_eq!(
            list_tasks(dir.path()).unwrap(),
            vec!["find_target".to_string(), "task1".to_string(), "task2".to_string()]
        );
    }

    #[test]
    fn task_exists_reports_invalid_json() {
        let dir = tempfile::tempdir().unwrap();
        let task_dir = dir.path().join("configs/rl/tasks/broken");
        std::fs::create_dir_all(&task_dir).unwrap();
        std::fs::write(task_dir.join("task.json"), "not json").unwrap();
        let err = task_exists(dir.path(), "broken").unwrap_err();
        assert!(matches!(err, TaskLookupError::InvalidJson { .. }));
    }

    mod algorithm_field_tests {
        use super::*;

        #[test]
        fn every_algorithm_field_has_a_unique_wire_name() {
            let names: Vec<&str> = ALL_ALGORITHM_FIELDS.iter().map(|f| f.wire_name()).collect();
            let unique: std::collections::HashSet<&str> = names.iter().copied().collect();
            assert_eq!(names.len(), unique.len());
        }

        fn all_ppo_fields_set() -> AlgorithmFields {
            AlgorithmFields {
                total_timesteps: Some(1_000_000),
                learning_rate: Some(0.0003),
                n_steps: Some(2048),
                n_epochs: Some(10),
                patience: Some(10),
                min_delta: Some(0.01),
            }
        }

        #[test]
        fn ppo_passes_with_all_six_fields_set() {
            assert!(validate_algorithm_fields("ppo", FlowType::Train, &all_ppo_fields_set()).is_empty());
        }

        /// Regression test for the bug this `SentinelConvention` split exists to fix: a `ppo`
        /// job with `patience: -1` / `min_delta: -1.0` - exactly what the scaffolded `train.yml`
        /// template sets (see `mecha10-templates`' `rl_bundle.rs::FIXTURE_TRAIN`) to mean "no
        /// early-stopping limit" - must validate as *set*, not get reported as missing. Before
        /// this fix, every caller stripped `-1`/`-1.0` off every numeric field (including these
        /// two) before this crate ever saw the value, so a correctly-configured ppo job failed
        /// validation. See PR #1639/#1641 for the reverted ad hoc attempt at this fix.
        #[test]
        fn ppo_passes_with_patience_and_min_delta_at_their_disabled_sentinel_value() {
            let fields = AlgorithmFields {
                patience: Some(-1),
                min_delta: Some(-1.0),
                ..all_ppo_fields_set()
            };
            assert!(
                validate_algorithm_fields("ppo", FlowType::Train, &fields).is_empty(),
                "patience: -1 / min_delta: -1.0 are meaningful 'disabled' values for ppo, not absence"
            );
        }

        /// The flip side of the regression above: fields with no sentinel convention
        /// (`total_timesteps` here) must still treat `-1` as absent, the same as actually unset -
        /// this guards against over-generalizing "sentinels are fine everywhere" when fixing the
        /// `patience`/`min_delta` bug.
        #[test]
        fn ppo_still_rejects_total_timesteps_set_to_the_placeholder_sentinel_value() {
            let fields = AlgorithmFields {
                total_timesteps: Some(-1),
                ..all_ppo_fields_set()
            };
            let missing = validate_algorithm_fields("ppo", FlowType::Train, &fields);
            assert_eq!(
                missing,
                vec![MissingAlgorithmField {
                    field: AlgorithmField::TotalTimesteps,
                    algorithm: "ppo".to_string()
                }],
                "total_timesteps: -1 has no meaningful-sentinel convention, so it must still count as absent"
            );
        }

        #[test]
        fn ppo_requires_each_field_individually() {
            for field in ALL_ALGORITHM_FIELDS {
                let mut fields = all_ppo_fields_set();
                match field {
                    AlgorithmField::TotalTimesteps => fields.total_timesteps = None,
                    AlgorithmField::LearningRate => fields.learning_rate = None,
                    AlgorithmField::NSteps => fields.n_steps = None,
                    AlgorithmField::NEpochs => fields.n_epochs = None,
                    AlgorithmField::Patience => fields.patience = None,
                    AlgorithmField::MinDelta => fields.min_delta = None,
                }
                let missing = validate_algorithm_fields("ppo", FlowType::Train, &fields);
                assert_eq!(
                    missing,
                    vec![MissingAlgorithmField {
                        field: *field,
                        algorithm: "ppo".to_string()
                    }],
                    "expected only {field:?} to be reported missing"
                );
            }
        }

        #[test]
        fn ppo_reports_every_missing_field_when_all_are_absent() {
            let missing = validate_algorithm_fields("ppo", FlowType::Train, &AlgorithmFields::default());
            assert_eq!(missing.len(), ALL_ALGORITHM_FIELDS.len());
        }

        #[test]
        fn ppo_resume_run_does_not_require_n_steps_but_still_requires_everything_else() {
            // A `ppo` resume-run restores the rollout length from the checkpoint it resumes, so
            // `n_steps` specifically is the one field it may omit. Only `n_steps` is carved out -
            // every other `ppo` hyperparameter is still required for a resume-run.
            let mut fields = all_ppo_fields_set();
            fields.n_steps = None;
            assert!(
                validate_algorithm_fields("ppo", FlowType::ResumeRun, &fields).is_empty(),
                "a ppo resume-run without n_steps should pass"
            );

            // ...but dropping total_timesteps (not carved out) must still fail, even for a
            // resume-run.
            fields.total_timesteps = None;
            let missing = validate_algorithm_fields("ppo", FlowType::ResumeRun, &fields);
            assert_eq!(
                missing,
                vec![MissingAlgorithmField {
                    field: AlgorithmField::TotalTimesteps,
                    algorithm: "ppo".to_string()
                }],
                "a ppo resume-run without total_timesteps should still fail (and n_steps stays exempt)"
            );
        }

        #[test]
        fn ppo_n_steps_carve_out_is_resume_run_only() {
            // The carve-out is specific to resume-run: on every other flow_type a ppo job still
            // requires n_steps.
            for flow_type in [
                FlowType::Train,
                FlowType::Tune,
                FlowType::TuneThenTrain,
                FlowType::TrainFromTune,
            ] {
                let mut fields = all_ppo_fields_set();
                fields.n_steps = None;
                let missing = validate_algorithm_fields("ppo", flow_type, &fields);
                assert_eq!(
                    missing,
                    vec![MissingAlgorithmField {
                        field: AlgorithmField::NSteps,
                        algorithm: "ppo".to_string()
                    }],
                    "expected n_steps to be required for ppo on {flow_type}"
                );
            }
        }

        fn off_policy_fields_set() -> AlgorithmFields {
            AlgorithmFields {
                total_timesteps: Some(1_000_000),
                learning_rate: Some(0.0003),
                n_steps: None,
                n_epochs: None,
                patience: Some(10),
                min_delta: Some(0.01),
            }
        }

        #[test]
        fn sac_td3_a2c_pass_with_their_four_fields_set_and_no_n_steps_or_n_epochs() {
            for algorithm in ["sac", "td3", "a2c"] {
                assert!(
                    validate_algorithm_fields(algorithm, FlowType::Train, &off_policy_fields_set()).is_empty(),
                    "expected {algorithm} to pass"
                );
            }
        }

        #[test]
        fn sac_td3_a2c_require_their_four_fields() {
            for algorithm in ["sac", "td3", "a2c"] {
                let missing = validate_algorithm_fields(algorithm, FlowType::Train, &AlgorithmFields::default());
                assert_eq!(missing.len(), 4, "expected 4 missing fields for {algorithm}");
                assert!(missing.iter().all(|m| m.algorithm == algorithm));
                for field in [
                    AlgorithmField::TotalTimesteps,
                    AlgorithmField::LearningRate,
                    AlgorithmField::Patience,
                    AlgorithmField::MinDelta,
                ] {
                    assert!(
                        missing.iter().any(|m| m.field == field),
                        "expected {field:?} to be reported missing for {algorithm}"
                    );
                }
            }
        }

        #[test]
        fn sac_td3_a2c_do_not_require_n_steps_or_n_epochs() {
            for algorithm in ["sac", "td3", "a2c"] {
                let mut fields = off_policy_fields_set();
                fields.n_steps = None;
                fields.n_epochs = None;
                let missing = validate_algorithm_fields(algorithm, FlowType::Train, &fields);
                assert!(!missing.iter().any(|m| m.field == AlgorithmField::NSteps));
                assert!(!missing.iter().any(|m| m.field == AlgorithmField::NEpochs));
            }
        }

        #[test]
        fn act_passes_with_none_of_these_fields_set() {
            assert!(validate_algorithm_fields("act", FlowType::Train, &AlgorithmFields::default()).is_empty());
        }

        #[test]
        fn unknown_or_unset_algorithm_passes_with_none_of_these_fields_set() {
            for algorithm in ["", "some-future-algorithm"] {
                assert!(
                    validate_algorithm_fields(algorithm, FlowType::Train, &AlgorithmFields::default()).is_empty(),
                    "expected {algorithm:?} to pass with no fields set"
                );
            }
        }
    }
}