vars: delete old var when changing share

When changing a password from non-shared to shared, we want to remove the old one

pkgs/clan-cli/clan_cli/tests/test_vars.py

@@ -1121,6 +1121,89 @@ def test_invalidation(
     assert value2 == value2_new
 
 
+@pytest.mark.with_core
+def test_share_mode_switch_regenerates_secret(
+    monkeypatch: pytest.MonkeyPatch,
+    flake_with_sops: ClanFlake,
+) -> None:
+    """
+    Test that switching a generator from share=false to share=true
+    causes the secret to be regenerated with a new value.
+    """
+    flake = flake_with_sops
+
+    config = flake.machines["my_machine"]
+    config["nixpkgs"]["hostPlatform"] = "x86_64-linux"
+
+    # Create a generator with share=false initially
+    my_generator = config["clan"]["core"]["vars"]["generators"]["my_generator"]
+    my_generator["share"] = False
+    my_generator["files"]["my_value"]["secret"] = False
+    my_generator["files"]["my_secret"]["secret"] = True
+    my_generator["script"] = (
+        'echo -n "public$RANDOM" > "$out"/my_value && echo -n "secret$RANDOM" > "$out"/my_secret'
+    )
+
+    flake.refresh()
+    monkeypatch.chdir(flake.path)
+
+    # Generate with share=false
+    cli.run(["vars", "generate", "--flake", str(flake.path), "my_machine"])
+
+    # Read the initial values
+    flake_obj = Flake(str(flake.path))
+    in_repo_store = in_repo.FactStore(flake=flake_obj)
+    sops_store = sops.SecretStore(flake=flake_obj)
+
+    generator_not_shared = Generator(
+        "my_generator", share=False, machine="my_machine", _flake=flake_obj
+    )
+
+    initial_public = in_repo_store.get(generator_not_shared, "my_value").decode()
+    initial_secret = sops_store.get(generator_not_shared, "my_secret").decode()
+
+    # Verify initial values exist and have expected format
+    assert initial_public.startswith("public")
+    assert initial_secret.startswith("secret")
+
+    # Now switch to share=true
+    my_generator["share"] = True
+    flake.refresh()
+
+    # Generate again with share=true
+    cli.run(["vars", "generate", "--flake", str(flake.path), "my_machine"])
+
+    # Read the new values with shared generator
+    generator_shared = Generator(
+        "my_generator", share=True, machine="my_machine", _flake=flake_obj
+    )
+
+    new_public = in_repo_store.get(generator_shared, "my_value").decode()
+    new_secret = sops_store.get(generator_shared, "my_secret").decode()
+
+    # Verify that both values have changed (regenerated)
+    assert new_public != initial_public, (
+        "Public value should be regenerated when switching to share=true"
+    )
+    assert new_secret != initial_secret, (
+        "Secret value should be regenerated when switching to share=true"
+    )
+
+    # Verify new values still have expected format
+    assert new_public.startswith("public")
+    assert new_secret.startswith("secret")
+
+    # Verify the old machine-specific secret no longer exists
+    assert not sops_store.exists(generator_not_shared, "my_secret"), (
+        "Machine-specific secret should be removed"
+    )
+
+    # Verify the new shared secret exists
+    assert sops_store.exists(generator_shared, "my_secret"), (
+        "Shared secret should exist"
+    )
+
+
 @pytest.mark.with_core
 def test_dynamic_invalidation(
     monkeypatch: pytest.MonkeyPatch,

pkgs/clan-cli/clan_cli/vars/_types.py

@@ -1,3 +1,4 @@
+import dataclasses
 import logging
 from abc import ABC, abstractmethod
 from collections.abc import Callable, Iterable
@@ -139,9 +140,18 @@ class StoreBase(ABC):
         var: "Var",
         value: bytes,
         is_migration: bool = False,
-    ) -> Path | None:
+    ) -> list[Path]:
         from clan_lib.machines.machines import Machine
 
+        changed_files: list[Path] = []
+
+        # if generator was switched from shared to per-machine or vice versa,
+        # remove the old var first
+        if self.exists(
+            gen := dataclasses.replace(generator, share=not generator.share), var.name
+        ):
+            changed_files += self.delete(gen, var.name)
+
         if self.exists(generator, var.name):
             if self.is_secret_store:
                 old_val = None
@@ -171,7 +181,8 @@ class StoreBase(ABC):
             log_info(
                 f"Var {generator.name}/{var.name} remains unchanged: {old_val_str}",
             )
-        return new_file
+        changed_files += [new_file] if new_file else []
+        return changed_files
 
     @abstractmethod
     def delete(self, generator: "Generator", name: str) -> Iterable[Path]:

pkgs/clan-cli/clan_cli/vars/generator.py

@@ -410,21 +410,20 @@ class Generator:
                             msg += f"  - {f.name}\n"
                     raise ClanError(msg)
                 if file.secret:
-                    file_path = machine.secret_vars_store.set(
+                    file_paths = machine.secret_vars_store.set(
                         self,
                         file,
                         secret_file.read_bytes(),
                     )
                     secret_changed = True
                 else:
-                    file_path = machine.public_vars_store.set(
+                    file_paths = machine.public_vars_store.set(
                         self,
                         file,
                         secret_file.read_bytes(),
                     )
                     public_changed = True
-                if file_path:
-                    files_to_commit.append(file_path)
+                files_to_commit.extend(file_paths)
 
             validation = self.validation()
             if validation is not None:

pkgs/clan-cli/clan_cli/vars/migration.py

@@ -58,24 +58,16 @@ def _migrate_file(
 
     if file.secret:
         old_value = machine.secret_facts_store.get(service_name, fact_name)
-        maybe_path = machine.secret_vars_store.set(
-            generator,
-            file,
-            old_value,
-            is_migration=True,
+        paths_list = machine.secret_vars_store.set(
+            generator, file, old_value, is_migration=True
         )
-        if maybe_path:
-            paths.append(maybe_path)
+        paths.extend(paths_list)
     else:
         old_value = machine.public_facts_store.get(service_name, fact_name)
-        maybe_path = machine.public_vars_store.set(
-            generator,
-            file,
-            old_value,
-            is_migration=True,
+        paths_list = machine.public_vars_store.set(
+            generator, file, old_value, is_migration=True
         )
-        if maybe_path:
-            paths.append(maybe_path)
+        paths.extend(paths_list)
 
     return paths
 

pkgs/clan-cli/clan_cli/vars/set.py

@@ -28,10 +28,10 @@ def set_var(machine: str | Machine, var: str | Var, value: bytes, flake: Flake)
         _var = get_machine_var(_machine, var)
     else:
         _var = var
-    path = _var.set(value)
-    if path:
+    paths = _var.set(value)
+    if paths:
         commit_files(
-            [path],
+            paths,
             _machine.flake_dir,
             f"Update var {_var.id} for machine {_machine.name}",
         )

pkgs/clan-cli/clan_cli/vars/var.py

@@ -46,7 +46,7 @@ class Var:
         except UnicodeDecodeError:
             return "<binary blob>"
 
-    def set(self, value: bytes) -> Path | None:
+    def set(self, value: bytes) -> list[Path]:
         assert self._store is not None
         assert self._generator is not None
         return self._store.set(self._generator, self, value)