Simplify recent CLI additions: dedup, fix TOCTOU, remove wrappers

- Extract git_repo_root() helper in init.rs (was duplicated between
  run_init and run_deinit)
- Fix TOCTOU in run_deinit: remove .exists() check, handle NotFound
  from remove_file directly
- Change dotenv::remove_env_key() to return Option<String> so callers
  don't need to separately parse the file to check key existence
- Remove merge_env wrapper in install.rs, call shared function directly
- Remove duplicate merge_env tests from install.rs (already in dotenv.rs)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

lib/crates/fabro-cli/src/commands/secret.rs

@@ -68,15 +68,15 @@ pub fn rm_command(args: &SecretRmArgs) -> Result<()> {
         }
         Err(e) => bail!("failed to read {}: {e}", path.display()),
     };
-    // Check the key actually exists before removing
-    let pairs = dotenv::parse_env(&contents);
-    if !pairs.iter().any(|(k, _)| k == &args.key) {
-        bail!("secret not found: {}", args.key);
-    }
     let updated = dotenv::remove_env_key(&contents, &args.key);
-    dotenv::write_env_file(&path, &updated)?;
-    eprintln!("Removed {}", args.key);
-    Ok(())
+    match updated {
+        Some(new_contents) => {
+            dotenv::write_env_file(&path, &new_contents)?;
+            eprintln!("Removed {}", args.key);
+            Ok(())
+        }
+        None => bail!("secret not found: {}", args.key),
+    }
 }
 
 pub fn set_command(args: &SecretSetArgs) -> Result<()> {

lib/crates/fabro-cli/src/init.rs

@@ -1,21 +1,23 @@
 use anyhow::{bail, Context, Result};
 use std::path::PathBuf;
 
-pub async fn run_init() -> Result<()> {
+fn git_repo_root() -> Result<PathBuf> {
     let output = std::process::Command::new("git")
         .args(["rev-parse", "--show-toplevel"])
         .output()
         .context("failed to run git")?;
-
     if !output.status.success() {
         bail!("not a git repository — run `git init` first");
     }
-
-    let repo_root = PathBuf::from(
+    Ok(PathBuf::from(
         String::from_utf8(output.stdout)
             .context("git output was not valid UTF-8")?
             .trim(),
-    );
+    ))
+}
+
+pub async fn run_init() -> Result<()> {
+    let repo_root = git_repo_root()?;
 
     let fabro_toml = repo_root.join("fabro.toml");
     if fabro_toml.exists() {
@@ -111,31 +113,20 @@ draft = true
 }
 
 pub fn run_deinit() -> Result<()> {
-    let output = std::process::Command::new("git")
-        .args(["rev-parse", "--show-toplevel"])
-        .output()
-        .context("failed to run git")?;
-
-    if !output.status.success() {
-        bail!("not a git repository");
-    }
-
-    let repo_root = PathBuf::from(
-        String::from_utf8(output.stdout)
-            .context("git output was not valid UTF-8")?
-            .trim(),
-    );
+    let repo_root = git_repo_root()?;
 
     let fabro_toml = repo_root.join("fabro.toml");
-    if !fabro_toml.exists() {
-        bail!("not initialized — fabro.toml not found");
-    }
 
     let green = console::Style::new().green();
     let dim = console::Style::new().dim();
 
-    std::fs::remove_file(&fabro_toml)
-        .with_context(|| format!("failed to remove {}", fabro_toml.display()))?;
+    match std::fs::remove_file(&fabro_toml) {
+        Ok(()) => {}
+        Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
+            bail!("not initialized — fabro.toml not found");
+        }
+        Err(e) => bail!("failed to remove {}: {e}", fabro_toml.display()),
+    }
     eprintln!(
         "  {} {}",
         green.apply_to("✔"),

lib/crates/fabro-cli/src/install.rs

@@ -199,14 +199,6 @@ ca = "~/.fabro/certs/ca.crt"
     )
 }
 
-// ---------------------------------------------------------------------------
-// .env merge (delegates to fabro_config::dotenv)
-// ---------------------------------------------------------------------------
-
-fn merge_env(existing: &str, new_vars: &[(&str, &str)]) -> String {
-    fabro_config::dotenv::merge_env(existing, new_vars)
-}
-
 // ---------------------------------------------------------------------------
 // Provider key URLs
 // ---------------------------------------------------------------------------
@@ -549,7 +541,7 @@ fn write_env_file(arc_dir: &Path, env_pairs: &[(String, String)], s: &Styles) ->
         .iter()
         .map(|(k, v)| (k.as_str(), v.as_str()))
         .collect();
-    let merged = merge_env(&existing, &refs);
+    let merged = fabro_config::dotenv::merge_env(&existing, &refs);
     fabro_config::dotenv::write_env_file(&env_path, &merged)?;
     eprintln!(
         "  {}",
@@ -1160,39 +1152,6 @@ mod tests {
         assert_eq!(tls.ca, PathBuf::from("~/.fabro/certs/ca.crt"));
     }
 
-    // -- .env merge --
-
-    #[test]
-    fn merge_env_replaces_existing() {
-        let result = merge_env("FOO=old\nBAR=keep\n", &[("FOO", "new"), ("BAZ", "added")]);
-        assert!(result.contains("FOO=new"));
-        assert!(result.contains("BAR=keep"));
-        assert!(result.contains("BAZ=added"));
-    }
-
-    #[test]
-    fn merge_env_empty_existing() {
-        let result = merge_env("", &[("FOO", "bar"), ("BAZ", "qux")]);
-        assert!(result.contains("FOO=bar"));
-        assert!(result.contains("BAZ=qux"));
-    }
-
-    #[test]
-    fn merge_env_preserves_comments_and_blanks() {
-        let existing = "# A comment\n\nFOO=old\n# Another\nBAR=keep\n";
-        let result = merge_env(existing, &[("FOO", "new")]);
-        assert!(result.contains("# A comment"));
-        assert!(result.contains("# Another"));
-        assert!(result.contains("FOO=new"));
-        assert!(result.contains("BAR=keep"));
-    }
-
-    #[test]
-    fn merge_env_full_scenario() {
-        let result = merge_env("FOO=old\nBAR=keep", &[("FOO", "new"), ("BAZ", "added")]);
-        assert_eq!(result, "FOO=new\nBAR=keep\nBAZ=added\n");
-    }
-
     // -- Provider key URLs --
 
     #[test]

lib/crates/fabro-config/src/dotenv.rs

@@ -65,23 +65,29 @@ pub fn merge_env(existing: &str, new_vars: &[(&str, &str)]) -> String {
 }
 
 /// Remove a key from an env file's raw contents. Comments and blank lines are
-/// preserved. Returns the new file contents.
-pub fn remove_env_key(contents: &str, key_to_remove: &str) -> String {
+/// preserved. Returns `Some(new_contents)` if the key was found and removed,
+/// or `None` if the key was not present.
+pub fn remove_env_key(contents: &str, key_to_remove: &str) -> Option<String> {
     let mut result_lines: Vec<String> = Vec::new();
+    let mut found = false;
     for line in contents.lines() {
         if let Some(eq_pos) = line.find('=') {
             let key = line[..eq_pos].trim();
             if !key.is_empty() && !key.starts_with('#') && key == key_to_remove {
+                found = true;
                 continue;
             }
         }
         result_lines.push(line.to_string());
     }
+    if !found {
+        return None;
+    }
     let mut result = result_lines.join("\n");
     if !result.ends_with('\n') {
         result.push('\n');
     }
-    result
+    Some(result)
 }
 
 /// Write content to the env file, creating parent directories if needed.
@@ -180,29 +186,27 @@ mod tests {
 
     #[test]
     fn remove_env_key_removes_target() {
-        let result = remove_env_key("FOO=bar\nBAZ=qux\n", "FOO");
+        let result = remove_env_key("FOO=bar\nBAZ=qux\n", "FOO").unwrap();
         assert!(!result.contains("FOO"));
         assert!(result.contains("BAZ=qux"));
     }
 
     #[test]
     fn remove_env_key_preserves_others() {
-        let result = remove_env_key("A=1\nB=2\nC=3\n", "B");
+        let result = remove_env_key("A=1\nB=2\nC=3\n", "B").unwrap();
         assert!(result.contains("A=1"));
         assert!(!result.contains("B=2"));
         assert!(result.contains("C=3"));
     }
 
     #[test]
-    fn remove_env_key_handles_missing_key() {
-        let original = "FOO=bar\n";
-        let result = remove_env_key(original, "MISSING");
-        assert_eq!(result, original);
+    fn remove_env_key_returns_none_for_missing_key() {
+        assert!(remove_env_key("FOO=bar\n", "MISSING").is_none());
     }
 
     #[test]
     fn remove_env_key_preserves_comments() {
-        let result = remove_env_key("# keep me\nFOO=bar\nBAZ=qux\n", "FOO");
+        let result = remove_env_key("# keep me\nFOO=bar\nBAZ=qux\n", "FOO").unwrap();
         assert!(result.contains("# keep me"));
         assert!(result.contains("BAZ=qux"));
         assert!(!result.contains("FOO"));