fix(config): wire dead validate() functions and fix migrate idempotency guards (#6058)

Config::validate() now calls 7 subsystem validate() functions that had
real invariant checks but were unreachable from the production config-load
path, most severely validate_pool() which two code comments elsewhere
treat as a load-bearing guarantee against an empty [[llm.providers]] list.
Config::default() now seeds one provider so --dump-config-defaults and the
no-config-file TUI-remote fallback stay self-consistent with the newly
enforced invariant.

Also fixes two --migrate-config --in-place steps that re-appended their
advisory block on every run instead of converging after one, same defect
class as #5945: the mcp steps anchored on a raw substring that matched
inside a commented catch-all stub, and the memory query-bias step guarded
on the wrong (uncommented) form of its own commented output.

Closes #5932
Closes #6018

Author: bug-ops

CHANGELOG.md

@@ -89,6 +89,34 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ### Fixed
 
+- `fix(config)`: `Config::validate()` now calls 7 subsystem `validate()` functions that existed
+  with real invariant checks but were unreachable from the production config-load path — only
+  their own unit tests exercised them (#5932). `validate_pool()` was the most severe gap: two
+  code comments elsewhere in the codebase state, verbatim, that it "rejects an empty
+  `[[llm.providers]]` list at config-validation time" and treat that as a load-bearing guarantee,
+  but it was never actually invoked outside its own test module — a config with zero providers,
+  duplicate provider names, or multiple `default = true` entries previously loaded and validated
+  without error. The other six wired checks: `LlmConfig::validate_stt()` (dangling
+  `[llm.stt].provider` reference), `TrajectorySentinelConfig::validate()` (inverted risk
+  thresholds), `GatewayConfig::validate()`, `UtilityScoringConfig::validate()`,
+  `FidelityConfig::validate()`, and `AconConfig::validate()`. Also added `#[must_use]` to
+  `LlmConfig::validate_stt()` (missed by the earlier #4943/#4963 sweep since added afterward).
+  Because `Config::default()` had an empty provider pool, wiring `validate_pool()` made
+  `Config::default().validate()` fail, which in turn broke `--dump-config-defaults` and the
+  no-config-file fallback in `zeph --tui --connect`; fixed by seeding `Config::default()` with one
+  `ProviderEntry::default()` (`type = "ollama"`), matching the shipped `config/default.toml`
+  reference, which was already self-consistent.
+- `fix(config)`: two more `--migrate-config --in-place` steps re-appended their advisory block on
+  every run instead of converging after one, same defect class as #5945 (#6018). The
+  `mcp`/`mcp.elicitation`/`mcp` max-connect-attempts/retry-and-tool-timeout steps anchored on a
+  raw `toml_src.contains("[mcp]\n")` substring, which also matches inside a *commented* `# [mcp]`
+  stub left by the top-level migrator's catch-all pass when `[mcp]` was absent on a prior run —
+  now gated by `section_header_present(toml_src, "mcp")`, which correctly excludes commented
+  headers. `migrate_memory_retrieval_query_bias`'s idempotency guard checked for an *uncommented*
+  `query_bias_correction` field while the step only ever writes a *commented* advisory line, so
+  the guard never matched its own prior output; changed to a `contains` check on the raw source
+  (and switched to `section_header_present` for its section-presence check, for consistency with
+  the sibling `[memory.hebbian]` steps fixed in #5945).
 - `fix(tui)`: the durable executions overlay panel (`D` key) no longer bleeds stray glyphs from
   whatever widget last drew into the same sidebar `Rect` earlier in the frame (e.g. a trailing
   `e> t` fragment left over from the subagents/plan/security view rendered underneath) — `Clear`

crates/zeph-config/src/loader.rs

@@ -85,6 +85,29 @@ impl Config {
         self.acp
             .validate_auth_clients()
             .map_err(ConfigError::Validation)?;
+        // Provider pool: empty pool, duplicate names, and multiple `default = true`
+        // entries. Load-bearing guarantee relied on (verbatim) by
+        // `Agent::resolve_pool_entry_provider` (tier_loop.rs) and `arise.rs` — both assume
+        // a genuinely empty pool never occurs for a fully constructed production `Agent`.
+        crate::providers::validate_pool(&self.llm.providers)?;
+        self.llm.validate_stt()?;
+        self.security
+            .trajectory
+            .validate()
+            .map_err(ConfigError::Validation)?;
+        self.gateway.validate().map_err(ConfigError::Validation)?;
+        self.tools
+            .utility
+            .validate()
+            .map_err(ConfigError::Validation)?;
+        if let Some(fidelity) = &self.memory.fidelity {
+            fidelity.validate().map_err(ConfigError::Validation)?;
+        }
+        self.memory
+            .compression
+            .acon
+            .validate()
+            .map_err(ConfigError::Validation)?;
         Ok(())
     }
 
@@ -1091,4 +1114,167 @@ weight = 0.3
             "unexpected error: {err}"
         );
     }
+
+    // ── #5932: 7 previously-dead validate() functions now wired into Config::validate() ──────
+
+    #[test]
+    fn validate_rejects_empty_provider_pool() {
+        // This is the most severe gap from #5932: `validate_pool` was documented (verbatim)
+        // as a load-bearing guarantee by tier_loop.rs/arise.rs but was never actually wired
+        // in. `Config::default()` itself now seeds one provider (critic S1 follow-up, so
+        // `--dump-config-defaults` output stays self-consistent) — clear it explicitly to
+        // exercise the empty-pool branch.
+        let mut cfg = Config::default();
+        cfg.llm.providers.clear();
+        let err = cfg.validate().unwrap_err().to_string();
+        assert!(
+            err.contains("at least one LLM provider"),
+            "expected empty-pool error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_rejects_duplicate_provider_names() {
+        let mut cfg = Config::default();
+        cfg.llm.providers.push(crate::providers::ProviderEntry {
+            provider_type: crate::providers::ProviderKind::Ollama,
+            ..Default::default()
+        });
+        let err = cfg.validate().unwrap_err().to_string();
+        assert!(
+            err.contains("duplicate provider name"),
+            "expected duplicate-name error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_rejects_multiple_default_providers() {
+        let mut cfg = Config::default();
+        cfg.llm.providers[0].default = true;
+        cfg.llm.providers.push(crate::providers::ProviderEntry {
+            provider_type: crate::providers::ProviderKind::Ollama,
+            name: Some("second".into()),
+            default: true,
+            ..Default::default()
+        });
+        let err = cfg.validate().unwrap_err().to_string();
+        assert!(
+            err.contains("default = true"),
+            "expected multiple-default error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_rejects_stt_provider_pointing_at_nonexistent_provider() {
+        let mut cfg = Config::default();
+        cfg.llm.stt = Some(crate::providers::SttConfig {
+            provider: crate::providers::ProviderName::new("ghost"),
+            language: crate::providers::default_stt_language(),
+        });
+        let err = cfg.validate().unwrap_err().to_string();
+        assert!(
+            err.contains("[llm.stt].provider") && err.contains("ghost"),
+            "expected stt-provider-mismatch error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_accepts_stt_provider_matching_existing_provider() {
+        let mut cfg = Config::default();
+        cfg.llm.stt = Some(crate::providers::SttConfig {
+            provider: crate::providers::ProviderName::new("ollama"),
+            language: crate::providers::default_stt_language(),
+        });
+        assert!(cfg.validate().is_ok());
+    }
+
+    #[test]
+    fn validate_rejects_trajectory_sentinel_inverted_thresholds() {
+        let mut cfg = Config::default();
+        cfg.security.trajectory.elevated_at = 0.9;
+        cfg.security.trajectory.high_at = 0.5;
+        let err = cfg.validate().unwrap_err().to_string();
+        assert!(
+            err.contains("elevated_at") && err.contains("high_at"),
+            "expected trajectory threshold-ordering error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_rejects_gateway_invalid_webhook_timeout() {
+        // `rate_limit` and `max_body_size` are already covered by `validate_scalar_bounds`
+        // (runs earlier in the pipeline), so testing those wouldn't prove
+        // `GatewayConfig::validate()` is actually wired in — it would pass identically on
+        // pre-#5932 code (critic-flagged shadowing, S2). `webhook_send_timeout_secs` is the
+        // one field uniquely reachable only through the new call.
+        let mut cfg = Config::default();
+        cfg.gateway.webhook_send_timeout_secs = 0;
+        let err = cfg.validate().unwrap_err().to_string();
+        assert!(
+            err.contains("webhook_send_timeout_secs"),
+            "expected gateway webhook_send_timeout_secs error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_rejects_negative_utility_scoring_weight() {
+        let mut cfg = Config::default();
+        cfg.tools.utility.gain_weight = -1.0;
+        let err = cfg.validate().unwrap_err().to_string();
+        assert!(
+            err.contains("gain_weight"),
+            "expected utility-scoring weight error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_rejects_fidelity_threshold_ordering() {
+        let mut cfg = Config::default();
+        cfg.memory.fidelity = Some(crate::fidelity::FidelityConfig {
+            full_threshold: 0.2,
+            compressed_threshold: 0.5,
+            ..Default::default()
+        });
+        let err = cfg.validate().unwrap_err().to_string();
+        assert!(
+            err.contains("full_threshold") && err.contains("compressed_threshold"),
+            "expected fidelity threshold-ordering error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_accepts_absent_fidelity_config() {
+        let cfg = Config::default();
+        assert!(cfg.memory.fidelity.is_none());
+        assert!(cfg.validate().is_ok());
+    }
+
+    #[test]
+    fn validate_rejects_acon_inverted_thresholds() {
+        let mut cfg = Config::default();
+        cfg.memory.compression.acon.passthrough_threshold = 5000;
+        cfg.memory.compression.acon.summarize_threshold = 1000;
+        let err = cfg.validate().unwrap_err().to_string();
+        assert!(
+            err.contains("passthrough_threshold") && err.contains("summarize_threshold"),
+            "expected acon threshold-ordering error, got: {err}"
+        );
+    }
+
+    /// Regression test (critic S1): `Config::default()` must itself satisfy `validate_pool`
+    /// so `--dump-config-defaults` (which serializes `Config::default()` verbatim,
+    /// `src/runner.rs`) emits a config that `zeph --config <dump>` can actually load and
+    /// validate, rather than a self-inconsistent onboarding trap.
+    #[test]
+    fn dump_defaults_output_is_self_consistent_and_validates() {
+        assert!(Config::default().validate().is_ok());
+
+        let dumped = Config::dump_defaults().expect("dump defaults");
+        assert!(
+            dumped.contains("[[llm.providers]]"),
+            "dumped defaults must include an active provider entry, got:\n{dumped}"
+        );
+        let reparsed: Config = toml::from_str(&dumped).expect("reparse dumped defaults");
+        assert!(reparsed.validate().is_ok());
+    }
 }

crates/zeph-config/src/migrate/mcp.rs

@@ -7,7 +7,7 @@
 //! the [`Migration`](super::Migration) trait, and the [`MIGRATIONS`](super::MIGRATIONS)
 //! registry remain in the parent module.
 
-use super::{MigrateError, MigrationResult};
+use super::{MigrateError, MigrationResult, section_header_present};
 
 /// Migrate `[[mcp.servers]]` entries to add `trust_level = "trusted"` for any entry
 /// that lacks an explicit `trust_level`.
@@ -79,30 +79,31 @@ pub fn migrate_mcp_trust_levels(toml_src: &str) -> Result<MigrationResult, Migra
 ///
 /// All elicitation fields have `#[serde(default)]` so existing configs parse without changes.
 ///
+/// Idempotent: the guard for the anchor section checks for an *active* `[mcp]` header via
+/// [`section_header_present`], not a naive substring match on `"[mcp]\n"` — the latter also
+/// matches inside a commented `# [mcp]` header (e.g. the stub the `ConfigMigrator` catch-all
+/// pass writes when the whole section is absent), which would splice this step's advisory
+/// comment into the middle of that commented block on a later run instead of staying a no-op
+/// (#6018).
+///
 /// # Errors
 ///
 /// Returns `MigrateError::Parse` if the TOML cannot be parsed.
 pub fn migrate_mcp_elicitation_config(toml_src: &str) -> Result<MigrationResult, MigrateError> {
-    // Idempotency: check for any elicitation key presence.
-    if toml_src.contains("elicitation_enabled") || toml_src.contains("# elicitation_enabled") {
-        return Ok(MigrationResult {
-            output: toml_src.to_owned(),
-            changed_count: 0,
-            sections_changed: Vec::new(),
-        });
-    }
-
-    // Only inject under an existing [mcp] section.
-    if !toml_src.contains("[mcp]") {
+    // Idempotency: the written form is always commented, so a plain substring check catches
+    // both the active and commented form of the key.
+    if toml_src.contains("elicitation_enabled") {
         return Ok(MigrationResult {
             output: toml_src.to_owned(),
             changed_count: 0,
             sections_changed: Vec::new(),
         });
     }
 
-    // Guard against configs that have `[mcp]` but with Windows line endings or at EOF.
-    if !toml_src.contains("[mcp]\n") {
+    // Only inject under an active [mcp] section. Also guards against Windows line endings
+    // or `[mcp]` at EOF, where the literal `"[mcp]\n"` anchor used by `replacen` below
+    // would not match.
+    if !section_header_present(toml_src, "mcp") || !toml_src.contains("[mcp]\n") {
         return Ok(MigrationResult {
             output: toml_src.to_owned(),
             changed_count: 0,
@@ -130,6 +131,11 @@ pub fn migrate_mcp_elicitation_config(toml_src: &str) -> Result<MigrationResult,
 /// configs omit it and get the default value of `3`. This migration surfaces the key
 /// as a comment so users can discover and tune it.
 ///
+/// Idempotent: the anchor guard requires an *active* `[mcp]` header via
+/// [`section_header_present`] rather than a naive substring match on `"[mcp]\n"`, which
+/// would also match inside a commented `# [mcp]` header (#6018, same defect shape as
+/// [`migrate_mcp_elicitation_config`]).
+///
 /// # Errors
 ///
 /// Returns `Ok` with unchanged output when the key is already present or `[mcp]` is absent.
@@ -142,7 +148,7 @@ pub fn migrate_mcp_max_connect_attempts(toml_src: &str) -> Result<MigrationResul
         });
     }
 
-    if !toml_src.contains("[mcp]\n") {
+    if !section_header_present(toml_src, "mcp") || !toml_src.contains("[mcp]\n") {
         return Ok(MigrationResult {
             output: toml_src.to_owned(),
             changed_count: 0,
@@ -167,14 +173,22 @@ pub fn migrate_mcp_max_connect_attempts(toml_src: &str) -> Result<MigrationResul
 /// Both keys have `#[serde(default)]` and require no user action; this migration surfaces them
 /// so operators can discover and tune the new retry and per-call timeout settings.
 ///
+/// Idempotent: the anchor guard requires an *active* `[mcp]` header via
+/// [`section_header_present`] rather than a naive substring match on `"[mcp]\n"`, which
+/// would also match inside a commented `# [mcp]` header (#6018, same defect shape as
+/// [`migrate_mcp_elicitation_config`]).
+///
 /// # Errors
 ///
 /// Returns `Ok` with unchanged output when either key is already present or `[mcp]` is absent.
 pub fn migrate_mcp_retry_and_tool_timeout(toml_src: &str) -> Result<MigrationResult, MigrateError> {
     let has_backoff = toml_src.contains("startup_retry_backoff_ms");
     let has_timeout = toml_src.contains("tool_timeout_secs");
 
-    if (has_backoff && has_timeout) || !toml_src.contains("[mcp]\n") {
+    if (has_backoff && has_timeout)
+        || !section_header_present(toml_src, "mcp")
+        || !toml_src.contains("[mcp]\n")
+    {
         return Ok(MigrationResult {
             output: toml_src.to_owned(),
             changed_count: 0,

crates/zeph-config/src/migrate/memory.rs

@@ -507,7 +507,11 @@ pub fn migrate_focus_auto_consolidate_min_window(
 /// (MM-F3, #3341) and is verified working in CI-604/CI-605. It is a no-op when the persona
 /// table is empty, so enabling it by default is safe.
 ///
-/// Idempotent: the section header (live or commented) suppresses re-injection.
+/// Idempotent: the step only ever writes `query_bias_correction` as a commented advisory
+/// line, so the guard checks for the key as a raw substring (matching both the active and
+/// commented form) rather than `starts_with` on trimmed lines, which never matches a
+/// `#`-prefixed line and would re-append the block on every run (#6018, same defect shape
+/// fixed for the `[memory.hebbian]` steps in #5945).
 ///
 /// # Errors
 ///
@@ -516,20 +520,21 @@ pub fn migrate_memory_retrieval_query_bias(
     toml_src: &str,
 ) -> Result<MigrationResult, MigrateError> {
     // Already handled by migrate_memory_retrieval_config if the whole section is absent.
-    // This step only splices the key into an existing [memory.retrieval] section.
-    if !toml_src.lines().any(|l| l.trim() == "[memory.retrieval]") {
+    // This step only splices the key into an existing [memory.retrieval] section. Uses
+    // `section_header_present` (not a bare exact-line match) so an inline-commented header
+    // (`[memory.retrieval]  # note`) is still recognized as active, matching the pattern
+    // used by the sibling `[memory.hebbian]` steps (critic M3, #5945 lesson).
+    if !section_header_present(toml_src, "memory.retrieval") {
         return Ok(MigrationResult {
             output: toml_src.to_owned(),
             changed_count: 0,
             sections_changed: Vec::new(),
         });
     }
 
-    // Idempotent: key already present (active or as comment).
-    if toml_src
-        .lines()
-        .any(|l| l.trim().starts_with("query_bias_correction"))
-    {
+    // Idempotent: key already present, active or commented — the step only ever writes
+    // the commented form, so a plain substring check catches both.
+    if toml_src.contains("query_bias_correction") {
         return Ok(MigrationResult {
             output: toml_src.to_owned(),
             changed_count: 0,