From 0e427dc4ba8677c9253af1d63163d4a4d635578b Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Sun, 19 Apr 2026 18:00:14 -0600 Subject: [PATCH] refactor: Migrate the vault/bare_init logic --- docs/implementation/PHASE-1-STEP-16c-NOTES.md | 196 ++++++++++++++++++ src/config/mod.rs | 12 +- src/main.rs | 3 +- src/vault/mod.rs | 18 +- 4 files changed, 208 insertions(+), 21 deletions(-) create mode 100644 docs/implementation/PHASE-1-STEP-16c-NOTES.md diff --git a/docs/implementation/PHASE-1-STEP-16c-NOTES.md b/docs/implementation/PHASE-1-STEP-16c-NOTES.md new file mode 100644 index 0000000..f59cefb --- /dev/null +++ b/docs/implementation/PHASE-1-STEP-16c-NOTES.md @@ -0,0 +1,196 @@ +# Phase 1 Step 16c — Implementation Notes + +## Status + +Done. + +## Plan reference + +- Parent plan: `docs/implementation/PHASE-1-STEP-16-NOTES.md` +- Sub-phase goal: "Migrate Vault onto AppState; Vault::init takes + `&AppConfig`" + +## Summary + +Changed `Vault::init(&Config)` and `Vault::init_bare()` to operate on +`AppConfig` instead of `Config`. Simplified `Vault::handle_vault_flags` +to accept a `&Vault` directly instead of extracting one from a Config +argument. Deleted the duplicate `Config::vault_password_file` method +(the canonical version lives on `AppConfig`). + +Vault is now fully Config-independent at the signature level. The +`Config.vault` runtime field still exists because it's populated +inside `Config::init` (for the current bridge-era flow), but nothing +about `Vault`'s API references `Config` anymore. The field itself +gets deleted in Step 16f when Config becomes a pure POJO. + +## What was changed + +### `src/vault/mod.rs` + +**Signature change:** +```rust +// Before +pub fn init(config: &Config) -> Self +// After +pub fn init(config: &AppConfig) -> Self +``` + +**`Vault::init_bare` now uses `AppConfig::default()`** instead of +`Config::default()` to get the default vault password file path. +Behavioral parity — both `.default().vault_password_file()` calls +resolve to the same path (the fallback from `gman::config`). + +**`Vault::handle_vault_flags` simplified:** +```rust +// Before +pub fn handle_vault_flags(cli: Cli, config: Config) -> Result<()> +// After +pub fn handle_vault_flags(cli: Cli, vault: &Vault) -> Result<()> +``` + +The old signature took a `Config` by value just to extract +`config.vault`. The new signature takes the Vault directly, which +decouples this function from Config entirely. Callers pass +`&config.vault` or equivalent. + +**Import updated:** +- `use crate::config::Config` removed +- `use crate::config::AppConfig` added + +### `src/config/mod.rs` + +**In `Config::init`:** +```rust +// Before +let vault = Vault::init(config); +// After +let vault = Vault::init(&config.to_app_config()); +``` + +This is a transitional call — `Config::init` builds a temporary +`AppConfig` via the bridge just to satisfy the new signature. This +temporary conversion disappears in Step 16e when `main.rs` stops +calling `Config::init` entirely and `AppConfig` is built first. + +**Deleted `Config::vault_password_file`** method. The identical body +lives on `AppConfig`. All callers go through AppConfig now. + +### `src/main.rs` + +**Vault-flags path:** +```rust +// Before +return Vault::handle_vault_flags(cli, Config::init_bare()?); +// After +let cfg = Config::init_bare()?; +return Vault::handle_vault_flags(cli, &cfg.vault); +``` + +This is a minor restructure — same observable behavior, but the +Vault is extracted from the Config and passed directly. Makes the +vault-flags path obvious about what it actually needs (a Vault, not +a Config). + +## Behavior parity + +- `Vault::init` reads `config.vault_password_file()` — identical + method on both Config and AppConfig (removed from Config in this + step, kept on AppConfig). +- Password file initialization (`ensure_password_file_initialized`) + still runs in `Vault::init` as before. +- `Vault::init_bare` fallback path resolves to the same default + password file location. +- `handle_vault_flags` operates on the same Vault instance either + way — just receives it directly instead of indirectly via Config. + +## Files modified + +- `src/vault/mod.rs` — imports, `init` signature, `init_bare` + fallback source, `handle_vault_flags` signature. +- `src/config/mod.rs` — transitional `to_app_config()` call in + `Config::init`; deleted duplicate `vault_password_file` method. +- `src/main.rs` — vault-flags path takes `&cfg.vault` directly. + +## Assumptions made + +1. **`AppConfig::default().vault_password_file()` behaves identically + to `Config::default().vault_password_file()`.** Verified by + comparing method bodies — identical logic, same fallback via + `gman::config::Config::local_provider_password_file()`. Tests + confirm 122 passing, no regressions. + +2. **Transitional `&config.to_app_config()` in `Config::init` is + acceptable.** The conversion happens once per Config::init call + — trivial cost for a non-hot path. Disappears entirely in 16e. + +3. **`handle_vault_flags` taking `&Vault` is a strict improvement.** + The old signature took `Config` by value (wasteful for a function + that only needed one field). The new signature is honest about + its dependency. + +4. **`Config.vault` field stays for now.** The `#[serde(skip)]` field + on Config still exists because `Config::init` populates it for + downstream Bridge flow consumers. Deletion deferred to 16f. + +## Open questions + +1. **Should `Vault::init` return `Result` instead of panicking?** + Currently uses `.expect("Failed to initialize password file")`. + The vault flags path can't do anything useful without a vault, + so panic vs early return is pragmatically equivalent. Leaving + as-is to minimize change surface in 16c. + +2. **Is `Config::init_bare` still needed after 16e?** It's called + from the oauth path in `main.rs`. In 16e we'll audit whether + those paths really need full Config init or just an AppConfig. + Deferred to 16e. + +## Verification + +- `cargo check` — clean, zero warnings +- `cargo clippy` — clean, zero warnings +- `cargo test` — 122 passing, zero failures +- Grep confirmation: + - `Vault::init(` has one caller (in `Config::init`); one + remaining via test path (none found — deferred to 16d/16e where + AppState::init will own vault construction) + - `Vault::init_bare(` has one caller (via `interpolate_secrets` + flow); no other references + - `Config::vault_password_file` — zero references anywhere + - `Vault::handle_vault_flags` — single caller in `main.rs`, + signature verified + +## Remaining work for Step 16 + +- **16d**: Build `AppState::init(app_config, ...).await` that takes + ownership of vault construction (replacing the current + `AppState { vault: cfg.vault.clone(), ... }` pattern). +- **16e**: Switch `main.rs` and all 15 `Config::init()` callers to + the new flow. +- **16f**: Delete `Config.vault` field, `Config::init`, bridge.rs, + and all remaining Config runtime fields. + +## Migration direction preserved + +Before 16c: +``` +Vault::init(&Config) ← tight coupling to Config +Vault::init_bare() ← uses Config::default() internally +handle_vault_flags(Cli, Config) ← takes Config, extracts vault +Config::vault_password_file() ← duplicate with AppConfig's +``` + +After 16c: +``` +Vault::init(&AppConfig) ← depends only on AppConfig +Vault::init_bare() ← uses AppConfig::default() internally +handle_vault_flags(Cli, &Vault) ← takes Vault directly +Config::vault_password_file() ← DELETED +``` + +The Vault module now has no Config dependency in its public API. +This means Step 16d can build `AppState::init` that calls +`Vault::init(&app_config)` without touching Config at all. It also +means `Config` is one step closer to being a pure POJO — one fewer +method on its surface, one fewer implicit dependency. diff --git a/src/config/mod.rs b/src/config/mod.rs index fb7409b..1d5d70f 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -356,7 +356,7 @@ impl Config { }; let setup = async |config: &mut Self| -> Result<()> { - let vault = Vault::init(config); + let vault = Vault::init(&config.to_app_config()); let (parsed_config, missing_secrets) = interpolate_secrets(&content, &vault); if !missing_secrets.is_empty() && !info_flag { @@ -411,16 +411,6 @@ impl Config { Ok(config) } - pub fn vault_password_file(&self) -> PathBuf { - match &self.vault_password_file { - Some(path) => match path.exists() { - true => path.clone(), - false => gman::config::Config::local_provider_password_file(), - }, - None => gman::config::Config::local_provider_password_file(), - } - } - pub fn sessions_dir(&self) -> PathBuf { match &self.agent { None => match env::var(get_env_name("sessions_dir")) { diff --git a/src/main.rs b/src/main.rs index 3aaaf7e..68ddfce 100644 --- a/src/main.rs +++ b/src/main.rs @@ -92,7 +92,8 @@ async fn main() -> Result<()> { } if vault_flags { - return Vault::handle_vault_flags(cli, Config::init_bare()?); + let cfg = Config::init_bare()?; + return Vault::handle_vault_flags(cli, &cfg.vault); } let abort_signal = create_abort_signal(); diff --git a/src/vault/mod.rs b/src/vault/mod.rs index e170075..2f691a8 100644 --- a/src/vault/mod.rs +++ b/src/vault/mod.rs @@ -5,7 +5,7 @@ pub use utils::create_vault_password_file; pub use utils::interpolate_secrets; use crate::cli::Cli; -use crate::config::Config; +use crate::config::AppConfig; use crate::vault::utils::ensure_password_file_initialized; use anyhow::{Context, Result}; use fancy_regex::Regex; @@ -26,7 +26,7 @@ pub type GlobalVault = Arc; impl Vault { pub fn init_bare() -> Self { - let vault_password_file = Config::default().vault_password_file(); + let vault_password_file = AppConfig::default().vault_password_file(); let local_provider = LocalProvider { password_file: Some(vault_password_file), git_branch: None, @@ -36,7 +36,7 @@ impl Vault { Self { local_provider } } - pub fn init(config: &Config) -> Self { + pub fn init(config: &AppConfig) -> Self { let vault_password_file = config.vault_password_file(); let mut local_provider = LocalProvider { password_file: Some(vault_password_file), @@ -130,25 +130,25 @@ impl Vault { Ok(secrets) } - pub fn handle_vault_flags(cli: Cli, config: Config) -> Result<()> { + pub fn handle_vault_flags(cli: Cli, vault: &Vault) -> Result<()> { if let Some(secret_name) = cli.add_secret { - config.vault.add_secret(&secret_name)?; + vault.add_secret(&secret_name)?; } if let Some(secret_name) = cli.get_secret { - config.vault.get_secret(&secret_name, true)?; + vault.get_secret(&secret_name, true)?; } if let Some(secret_name) = cli.update_secret { - config.vault.update_secret(&secret_name)?; + vault.update_secret(&secret_name)?; } if let Some(secret_name) = cli.delete_secret { - config.vault.delete_secret(&secret_name)?; + vault.delete_secret(&secret_name)?; } if cli.list_secrets { - config.vault.list_secrets(true)?; + vault.list_secrets(true)?; } Ok(())