refactor(setup): extract init logic from wizard into owning modules (#1210)

* refactor(setup): extract init logic from wizard into owning modules Move database, LLM model discovery, and secrets initialization logic out of the setup wizard and into their owning modules, following the CLAUDE.md principle that module-specific initialization must live in the owning module as a public factory function. Database (src/db/mod.rs, src/config/database.rs): - Add DatabaseConfig::from_postgres_url() and from_libsql_path() - Add connect_without_migrations() for connectivity testing - Add validate_postgres() returning structured PgDiagnostic results LLM (src/llm/models.rs — new file): - Extract 8 model-fetching functions from wizard.rs (~380 lines) - fetch_anthropic_models, fetch_openai_models, fetch_ollama_models, fetch_openai_compatible_models, build_nearai_model_fetch_config, and OpenAI sorting/filtering helpers Secrets (src/secrets/mod.rs): - Add resolve_master_key() unifying env var + keychain resolution - Add crypto_from_hex() convenience wrapper Wizard restructuring (src/setup/wizard.rs): - Replace cfg-gated db_pool/db_backend fields with generic db: Option<Arc<dyn Database>> + db_handles: Option<DatabaseHandles> - Delete 6 backend-specific methods (reconnect_postgres/libsql, test_database_connection_postgres/libsql, run_migrations_postgres/ libsql, create_postgres/libsql_secrets_store) - Simplify persist_settings, try_load_existing_settings, persist_session_to_db, init_secrets_context to backend-agnostic implementations using the new module factories - Eliminate all references to deadpool_postgres, PoolConfig, LibSqlBackend, Store::from_pool, refinery::embed_migrations Net: -878 lines from wizard, +395 lines in owning modules, +378 new. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * test(settings): add wizard re-run regression tests Add 10 tests covering settings preservation during wizard re-runs: - provider_only rerun preserves channels/embeddings/heartbeat - channels_only rerun preserves provider/model/embeddings - quick mode rerun preserves prior channels and heartbeat - full rerun same provider preserves model through merge - full rerun different provider clears model through merge - incremental persist doesn't clobber prior steps - switching DB backend allows fresh connection settings - merge preserves true booleans when overlay has default false - embeddings survive rerun that skips step 5 These cover the scenarios where re-running the wizard would previously risk resetting models, providers, or channel settings. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * refactor(setup): eliminate cfg(feature) gates from wizard methods Replace compile-time #[cfg(feature)] dispatch in the wizard with runtime dispatch via DatabaseBackend enum and cfg!() macro constants. - Merge step_database_postgres + step_database_libsql into step_database using runtime backend selection - Rewrite auto_setup_database without feature gates - Remove cfg(feature = "postgres") from mask_password_in_url (pure fn) - Remove cfg(feature = "postgres") from test_mask_password_in_url Only one internal #[cfg(feature = "postgres")] remains: guarding the call to db::validate_postgres() which is itself feature-gated. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * refactor(db): fold PG validation into connect_without_migrations Move PostgreSQL prerequisite validation (version >= 15, pgvector) from the wizard into connect_without_migrations() in the db module. The validation now returns DatabaseError directly with user-facing messages, eliminating the PgDiagnostic enum and the last #[cfg(feature)] gate from the wizard. The wizard's test_database_connection() is now a 5-line method that calls the db module factory and stores the result. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address PR review comments [skip-regression-check] - Use .as_ref().map() to avoid partial move of db_config.libsql_path (gemini-code-assist) - Default to available backend when DATABASE_BACKEND is invalid, not unconditionally to Postgres which may not be compiled (Copilot) - Match DatabaseBackend::Postgres explicitly instead of _ => wildcard in connect_with_handles, connect_without_migrations, and create_secrets_store to avoid silently routing LibSql configs through the Postgres path when libsql feature is disabled (Copilot) - Upgrade Ollama connection failure log from info to warn with the base URL for better visibility in wizard UX (Copilot) - Clarify crypto_from_hex doc: SecretsCrypto validates key length, not hex encoding (Copilot) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address zmanian's PR review feedback [skip-regression-check] - Update src/setup/README.md to reflect Arc<dyn Database> flow - Remove stale "Test PostgreSQL connection" doc comment - Replace unwrap_or(0) in validate_postgres with descriptive error - Add NearAiConfig::for_model_discovery() constructor - Narrow pub to pub(crate) for internal model helpers Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address Copilot review comments (quick-mode postgres gate, empty env vars) [skip-regression-check] - Gate DATABASE_URL auto-detection on POSTGRES_AVAILABLE in quick mode so libsql-only builds don't attempt a postgres connection - Match empty-env-var filtering in key source detection to align with resolve_master_key() behavior - Filter empty strings to None in DatabaseConfig::from_libsql_path() for turso_url/turso_token Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
2026-09-02 09:39:37 +00:00 · 2026-03-16 04:58:17 +00:00
parent 57c397bd50
commit e81fb7e5cb
9 changed files with 1388 additions and 1002 deletions
@@ -1747,4 +1747,503 @@ mod tests {
            "None selected_model should stay None"
        );
    }
+
+    // === Wizard re-run regression tests ===
+    //
+    // These tests simulate the merge ordering used by the wizard's `run()` method
+    // to verify that re-running the wizard (or a subset of steps) doesn't
+    // accidentally reset settings from prior runs.
+
+    /// Simulates `ironclaw onboard --provider-only` re-running on a fully
+    /// configured installation. Only provider + model should change; all
+    /// other settings (channels, embeddings, heartbeat) must survive.
+    #[test]
+    fn provider_only_rerun_preserves_unrelated_settings() {
+        // Prior completed run with everything configured
+        let prior = Settings {
+            onboard_completed: true,
+            database_backend: Some("libsql".to_string()),
+            libsql_path: Some("/home/user/.ironclaw/ironclaw.db".to_string()),
+            llm_backend: Some("openai".to_string()),
+            selected_model: Some("gpt-4o".to_string()),
+            embeddings: EmbeddingsSettings {
+                enabled: true,
+                provider: "openai".to_string(),
+                model: "text-embedding-3-small".to_string(),
+            },
+            channels: ChannelSettings {
+                http_enabled: true,
+                http_port: Some(8080),
+                signal_enabled: true,
+                signal_account: Some("+1234567890".to_string()),
+                wasm_channels: vec!["telegram".to_string()],
+                ..Default::default()
+            },
+            heartbeat: HeartbeatSettings {
+                enabled: true,
+                interval_secs: 900,
+                ..Default::default()
+            },
+            ..Default::default()
+        };
+        let db_map = prior.to_db_map();
+
+        // provider_only mode: reconnect_existing_db loads from DB,
+        // then user picks a new provider + model via step_inference_provider
+        let mut current = Settings::from_db_map(&db_map);
+
+        // Simulate step_inference_provider: user switches to anthropic
+        current.llm_backend = Some("anthropic".to_string());
+        current.selected_model = None; // cleared because backend changed
+
+        // Simulate step_model_selection: user picks a model
+        current.selected_model = Some("claude-sonnet-4-5".to_string());
+
+        // Verify: provider/model changed
+        assert_eq!(current.llm_backend.as_deref(), Some("anthropic"));
+        assert_eq!(current.selected_model.as_deref(), Some("claude-sonnet-4-5"));
+
+        // Verify: everything else preserved
+        assert!(current.channels.http_enabled, "HTTP channel must survive");
+        assert_eq!(current.channels.http_port, Some(8080));
+        assert!(current.channels.signal_enabled, "Signal must survive");
+        assert_eq!(
+            current.channels.wasm_channels,
+            vec!["telegram".to_string()],
+            "WASM channels must survive"
+        );
+        assert!(current.embeddings.enabled, "Embeddings must survive");
+        assert_eq!(current.embeddings.provider, "openai");
+        assert!(current.heartbeat.enabled, "Heartbeat must survive");
+        assert_eq!(current.heartbeat.interval_secs, 900);
+        assert_eq!(
+            current.database_backend.as_deref(),
+            Some("libsql"),
+            "DB backend must survive"
+        );
+    }
+
+    /// Simulates `ironclaw onboard --channels-only` re-running on a fully
+    /// configured installation. Only channel settings should change;
+    /// provider, model, embeddings, heartbeat must survive.
+    #[test]
+    fn channels_only_rerun_preserves_unrelated_settings() {
+        let prior = Settings {
+            onboard_completed: true,
+            database_backend: Some("postgres".to_string()),
+            database_url: Some("postgres://host/db".to_string()),
+            llm_backend: Some("anthropic".to_string()),
+            selected_model: Some("claude-sonnet-4-5".to_string()),
+            embeddings: EmbeddingsSettings {
+                enabled: true,
+                provider: "nearai".to_string(),
+                model: "text-embedding-3-small".to_string(),
+            },
+            heartbeat: HeartbeatSettings {
+                enabled: true,
+                interval_secs: 1800,
+                ..Default::default()
+            },
+            channels: ChannelSettings {
+                http_enabled: false,
+                wasm_channels: vec!["telegram".to_string()],
+                ..Default::default()
+            },
+            ..Default::default()
+        };
+        let db_map = prior.to_db_map();
+
+        // channels_only mode: reconnect_existing_db loads from DB
+        let mut current = Settings::from_db_map(&db_map);
+
+        // Simulate step_channels: user enables HTTP and adds discord
+        current.channels.http_enabled = true;
+        current.channels.http_port = Some(9090);
+        current.channels.wasm_channels = vec!["telegram".to_string(), "discord".to_string()];
+
+        // Verify: channels changed
+        assert!(current.channels.http_enabled);
+        assert_eq!(current.channels.http_port, Some(9090));
+        assert_eq!(current.channels.wasm_channels.len(), 2);
+
+        // Verify: everything else preserved
+        assert_eq!(current.llm_backend.as_deref(), Some("anthropic"));
+        assert_eq!(current.selected_model.as_deref(), Some("claude-sonnet-4-5"));
+        assert!(current.embeddings.enabled);
+        assert_eq!(current.embeddings.provider, "nearai");
+        assert!(current.heartbeat.enabled);
+        assert_eq!(current.heartbeat.interval_secs, 1800);
+    }
+
+    /// Simulates quick mode re-run on an installation that previously
+    /// completed a full setup. Quick mode only touches DB + security +
+    /// provider + model; channels, embeddings, heartbeat, extensions
+    /// should survive via the merge_from ordering.
+    #[test]
+    fn quick_mode_rerun_preserves_prior_channels_and_heartbeat() {
+        let prior = Settings {
+            onboard_completed: true,
+            database_backend: Some("libsql".to_string()),
+            libsql_path: Some("/home/user/.ironclaw/ironclaw.db".to_string()),
+            llm_backend: Some("openai".to_string()),
+            selected_model: Some("gpt-4o".to_string()),
+            channels: ChannelSettings {
+                http_enabled: true,
+                http_port: Some(8080),
+                signal_enabled: true,
+                wasm_channels: vec!["telegram".to_string()],
+                ..Default::default()
+            },
+            embeddings: EmbeddingsSettings {
+                enabled: true,
+                provider: "openai".to_string(),
+                model: "text-embedding-3-small".to_string(),
+            },
+            heartbeat: HeartbeatSettings {
+                enabled: true,
+                interval_secs: 600,
+                ..Default::default()
+            },
+            ..Default::default()
+        };
+        let db_map = prior.to_db_map();
+        let from_db = Settings::from_db_map(&db_map);
+
+        // Quick mode flow:
+        // 1. auto_setup_database sets DB fields
+        let step1 = Settings {
+            database_backend: Some("libsql".to_string()),
+            libsql_path: Some("/home/user/.ironclaw/ironclaw.db".to_string()),
+            ..Default::default()
+        };
+
+        // 2. try_load_existing_settings → merge DB → merge step1 on top
+        let mut current = step1.clone();
+        current.merge_from(&from_db);
+        current.merge_from(&step1);
+
+        // 3. step_inference_provider: user picks anthropic this time
+        current.llm_backend = Some("anthropic".to_string());
+        current.selected_model = None; // cleared because backend changed
+
+        // 4. step_model_selection: user picks model
+        current.selected_model = Some("claude-opus-4-6".to_string());
+
+        // Verify: provider/model updated
+        assert_eq!(current.llm_backend.as_deref(), Some("anthropic"));
+        assert_eq!(current.selected_model.as_deref(), Some("claude-opus-4-6"));
+
+        // Verify: channels, embeddings, heartbeat survived quick mode
+        assert!(
+            current.channels.http_enabled,
+            "HTTP channel must survive quick mode re-run"
+        );
+        assert_eq!(current.channels.http_port, Some(8080));
+        assert!(
+            current.channels.signal_enabled,
+            "Signal must survive quick mode re-run"
+        );
+        assert_eq!(
+            current.channels.wasm_channels,
+            vec!["telegram".to_string()],
+            "WASM channels must survive quick mode re-run"
+        );
+        assert!(
+            current.embeddings.enabled,
+            "Embeddings must survive quick mode re-run"
+        );
+        assert!(
+            current.heartbeat.enabled,
+            "Heartbeat must survive quick mode re-run"
+        );
+        assert_eq!(current.heartbeat.interval_secs, 600);
+    }
+
+    /// Full wizard re-run where user keeps the same provider. The model
+    /// selection from the prior run should be pre-populated (not reset).
+    ///
+    /// Regression: re-running with the same provider should preserve model.
+    #[test]
+    fn full_rerun_same_provider_preserves_model_through_merge() {
+        let prior = Settings {
+            onboard_completed: true,
+            database_backend: Some("postgres".to_string()),
+            database_url: Some("postgres://host/db".to_string()),
+            llm_backend: Some("anthropic".to_string()),
+            selected_model: Some("claude-sonnet-4-5".to_string()),
+            ..Default::default()
+        };
+        let db_map = prior.to_db_map();
+        let from_db = Settings::from_db_map(&db_map);
+
+        // Step 1: user keeps same DB
+        let step1 = Settings {
+            database_backend: Some("postgres".to_string()),
+            database_url: Some("postgres://host/db".to_string()),
+            ..Default::default()
+        };
+
+        let mut current = step1.clone();
+        current.merge_from(&from_db);
+        current.merge_from(&step1);
+
+        // After merge, prior settings recovered
+        assert_eq!(
+            current.llm_backend.as_deref(),
+            Some("anthropic"),
+            "Prior provider must be recovered from DB"
+        );
+        assert_eq!(
+            current.selected_model.as_deref(),
+            Some("claude-sonnet-4-5"),
+            "Prior model must be recovered from DB"
+        );
+
+        // Step 3: user picks same provider (anthropic)
+        // set_llm_backend_preserving_model checks if backend changed
+        let backend_changed = current.llm_backend.as_deref() != Some("anthropic");
+        current.llm_backend = Some("anthropic".to_string());
+        if backend_changed {
+            current.selected_model = None;
+        }
+
+        // Model should NOT be cleared since backend didn't change
+        assert_eq!(
+            current.selected_model.as_deref(),
+            Some("claude-sonnet-4-5"),
+            "Model must survive when re-selecting same provider"
+        );
+    }
+
+    /// Full wizard re-run where user switches provider. Model should be
+    /// cleared since the old model is invalid for the new backend.
+    #[test]
+    fn full_rerun_different_provider_clears_model_through_merge() {
+        let prior = Settings {
+            onboard_completed: true,
+            database_backend: Some("postgres".to_string()),
+            database_url: Some("postgres://host/db".to_string()),
+            llm_backend: Some("anthropic".to_string()),
+            selected_model: Some("claude-sonnet-4-5".to_string()),
+            ..Default::default()
+        };
+        let db_map = prior.to_db_map();
+        let from_db = Settings::from_db_map(&db_map);
+
+        // Step 1 merge
+        let step1 = Settings {
+            database_backend: Some("postgres".to_string()),
+            database_url: Some("postgres://host/db".to_string()),
+            ..Default::default()
+        };
+        let mut current = step1.clone();
+        current.merge_from(&from_db);
+        current.merge_from(&step1);
+
+        // Step 3: user switches to openai
+        let backend_changed = current.llm_backend.as_deref() != Some("openai");
+        assert!(backend_changed, "switching providers should be detected");
+        current.llm_backend = Some("openai".to_string());
+        if backend_changed {
+            current.selected_model = None;
+        }
+
+        assert_eq!(current.llm_backend.as_deref(), Some("openai"));
+        assert!(
+            current.selected_model.is_none(),
+            "Model must be cleared when switching providers"
+        );
+    }
+
+    /// Simulates incremental save correctness: persist_after_step after
+    /// Step 3 (provider) should not clobber settings set in Step 2 (security).
+    ///
+    /// The wizard persists the full settings object after each step. This
+    /// test verifies that incremental saves are idempotent for prior steps.
+    #[test]
+    fn incremental_persist_does_not_clobber_prior_steps() {
+        // After steps 1-2, settings has DB + security
+        let after_step2 = Settings {
+            database_backend: Some("libsql".to_string()),
+            secrets_master_key_source: KeySource::Keychain,
+            ..Default::default()
+        };
+
+        // persist_after_step saves to DB
+        let db_map_after_step2 = after_step2.to_db_map();
+
+        // Step 3 adds provider
+        let mut after_step3 = after_step2.clone();
+        after_step3.llm_backend = Some("openai".to_string());
+
+        // persist_after_step saves again — the full settings object
+        let db_map_after_step3 = after_step3.to_db_map();
+
+        // Reload from DB after step 3
+        let restored = Settings::from_db_map(&db_map_after_step3);
+
+        // Step 2's settings must survive step 3's persist
+        assert_eq!(
+            restored.secrets_master_key_source,
+            KeySource::Keychain,
+            "Step 2 security setting must survive step 3 persist"
+        );
+        assert_eq!(
+            restored.database_backend.as_deref(),
+            Some("libsql"),
+            "Step 1 DB setting must survive step 3 persist"
+        );
+        assert_eq!(
+            restored.llm_backend.as_deref(),
+            Some("openai"),
+            "Step 3 provider setting must be saved"
+        );
+
+        // Also verify that a partial step 2 reload doesn't regress
+        // (loading the step 2 snapshot and merging with step 3 state)
+        let from_step2_db = Settings::from_db_map(&db_map_after_step2);
+        let mut merged = after_step3.clone();
+        merged.merge_from(&from_step2_db);
+
+        assert_eq!(
+            merged.llm_backend.as_deref(),
+            Some("openai"),
+            "Step 3 provider must not be clobbered by step 2 snapshot merge"
+        );
+        assert_eq!(
+            merged.secrets_master_key_source,
+            KeySource::Keychain,
+            "Step 2 security must survive merge"
+        );
+    }
+
+    /// Switching database backend should allow fresh connection settings.
+    /// When user switches from postgres to libsql, the old database_url
+    /// should not prevent the new libsql_path from being used.
+    #[test]
+    fn switching_db_backend_allows_fresh_connection_settings() {
+        let prior = Settings {
+            database_backend: Some("postgres".to_string()),
+            database_url: Some("postgres://host/db".to_string()),
+            llm_backend: Some("openai".to_string()),
+            selected_model: Some("gpt-4o".to_string()),
+            ..Default::default()
+        };
+        let db_map = prior.to_db_map();
+        let from_db = Settings::from_db_map(&db_map);
+
+        // User picks libsql this time, wizard clears stale postgres settings
+        let step1 = Settings {
+            database_backend: Some("libsql".to_string()),
+            libsql_path: Some("/home/user/.ironclaw/ironclaw.db".to_string()),
+            database_url: None, // explicitly not set for libsql
+            ..Default::default()
+        };
+
+        let mut current = step1.clone();
+        current.merge_from(&from_db);
+        current.merge_from(&step1);
+
+        // libsql chosen
+        assert_eq!(current.database_backend.as_deref(), Some("libsql"));
+        assert_eq!(
+            current.libsql_path.as_deref(),
+            Some("/home/user/.ironclaw/ironclaw.db")
+        );
+
+        // Prior provider/model should survive (unrelated to DB switch)
+        assert_eq!(current.llm_backend.as_deref(), Some("openai"));
+        assert_eq!(current.selected_model.as_deref(), Some("gpt-4o"));
+
+        // Note: database_url from prior run persists in merge because
+        // step1.database_url is None (== default), so merge_from doesn't
+        // override it. This is expected — the .env writer decides which
+        // vars to emit based on database_backend. The stale URL is
+        // harmless because the libsql backend ignores it.
+        assert_eq!(
+            current.database_url.as_deref(),
+            Some("postgres://host/db"),
+            "stale database_url persists (harmless, ignored by libsql backend)"
+        );
+    }
+
+    /// Regression: merge_from must handle boolean fields correctly.
+    /// A prior run with heartbeat.enabled=true must not be reset to false
+    /// when merging with a Settings that has heartbeat.enabled=false (default).
+    #[test]
+    fn merge_preserves_true_booleans_when_overlay_has_default_false() {
+        let prior = Settings {
+            heartbeat: HeartbeatSettings {
+                enabled: true,
+                interval_secs: 600,
+                ..Default::default()
+            },
+            channels: ChannelSettings {
+                http_enabled: true,
+                signal_enabled: true,
+                ..Default::default()
+            },
+            ..Default::default()
+        };
+        let db_map = prior.to_db_map();
+        let from_db = Settings::from_db_map(&db_map);
+
+        // New wizard run only sets DB (everything else is default/false)
+        let step1 = Settings {
+            database_backend: Some("libsql".to_string()),
+            ..Default::default()
+        };
+
+        let mut current = step1.clone();
+        current.merge_from(&from_db);
+        current.merge_from(&step1);
+
+        // true booleans from prior run must survive
+        assert!(
+            current.heartbeat.enabled,
+            "heartbeat.enabled=true must not be reset to false by default overlay"
+        );
+        assert!(
+            current.channels.http_enabled,
+            "http_enabled=true must not be reset to false by default overlay"
+        );
+        assert!(
+            current.channels.signal_enabled,
+            "signal_enabled=true must not be reset to false by default overlay"
+        );
+        assert_eq!(current.heartbeat.interval_secs, 600);
+    }
+
+    /// Regression: embeddings settings (provider, model, enabled) must
+    /// survive a wizard re-run that doesn't touch step 5.
+    #[test]
+    fn embeddings_survive_rerun_that_skips_step5() {
+        let prior = Settings {
+            onboard_completed: true,
+            llm_backend: Some("nearai".to_string()),
+            selected_model: Some("qwen".to_string()),
+            embeddings: EmbeddingsSettings {
+                enabled: true,
+                provider: "nearai".to_string(),
+                model: "text-embedding-3-large".to_string(),
+            },
+            ..Default::default()
+        };
+        let db_map = prior.to_db_map();
+        let from_db = Settings::from_db_map(&db_map);
+
+        // Full re-run: step 1 only sets DB
+        let step1 = Settings {
+            database_backend: Some("libsql".to_string()),
+            ..Default::default()
+        };
+        let mut current = step1.clone();
+        current.merge_from(&from_db);
+        current.merge_from(&step1);
+
+        // Before step 5 (embeddings) runs, check that prior values are present
+        assert!(current.embeddings.enabled);
+        assert_eq!(current.embeddings.provider, "nearai");
+        assert_eq!(current.embeddings.model, "text-embedding-3-large");
+    }
 }