🐛 Various sandbox fixes, and a setup that runs itself

src/aiida_agents/cli/doctor.py

@@ -143,11 +143,14 @@ def _check_sandbox() -> _DiagnosticRow:
 
         sharing = profiles_sharing_storage(config, name)
         if sharing:
+            # Not `sandbox refresh`: that tears the sandbox down first, and
+            # teardown refuses a sandbox in exactly this state, so the advice
+            # would send the user to a command that cannot run.
             return _DiagnosticRow(
                 label,
                 False,
-                f"shares storage with {', '.join(sharing)}; deleting it would "
-                "destroy that data. Rebuild with `aiida-agents sandbox refresh`",
+                "; ".join(entry.describe() for entry in sharing)
+                + "; `aiida-agents sandbox check` spells out what to do",
             )
         return _DiagnosticRow(label, True, "own copy, shared with nothing")
     except Exception as exc:

src/aiida_agents/cli/sandbox.py

@@ -209,7 +209,7 @@ def check(profile: str) -> None:
     """
     from aiida.manage.configuration import get_config
 
-    from aiida_agents.sandbox.copy import profiles_sharing_storage
+    from aiida_agents.sandbox.copy import Overlap, profiles_sharing_storage
     from aiida_agents.sandbox.setup import verify_read_only
 
     config = get_config()
@@ -220,11 +220,26 @@ def check(profile: str) -> None:
 
     failures = profiles_sharing_storage(config, profile)
     if failures:
-        console.print(
-            f"[red]✗[/red] {profile!r} shares storage with "
-            f"{', '.join(repr(name) for name in failures)}. Deleting it would "
-            "destroy their data too. This must not be used as a sandbox."
-        )
+        # A proven overlap and a backend that cannot be read both stop the
+        # command, and saying which one it was is the difference between "your
+        # sandbox is pointed at your own database" and "there is a profile here
+        # I have never heard of". Only the first is the user's to fix.
+        proven = any(entry.overlap is Overlap.SHARED for entry in failures)
+        verdict = "must not be used" if proven else "cannot be cleared for use"
+        console.print(f"[red]✗[/red] {profile!r} {verdict} as a sandbox:")
+        for entry in failures:
+            console.print(f"    {entry.describe()}")
+        if proven:
+            console.print(
+                f"  Delete it with [cyan]verdi profile delete --keep-data "
+                f"{profile}[/cyan], then rebuild with [cyan]aiida-agents "
+                "sandbox init[/cyan]."
+            )
+        else:
+            console.print(
+                "  A storage backend this cannot read is treated as sharing "
+                "until it can be ruled out."
+            )
         raise SystemExit(1)
 
     console.print(f"[green]✓[/green] {profile!r} shares no storage with any profile")
@@ -279,9 +294,10 @@ def teardown(profile: str, yes: bool) -> None:
 
     sharing = profiles_sharing_storage(config, profile)
     if sharing:
+        reasons = "\n".join(f"  {entry.describe()}" for entry in sharing)
         raise click.ClickException(
-            f"{profile!r} shares storage with {', '.join(sharing)}. Refusing to "
-            "delete its data. Remove the profile by hand if you are certain."
+            f"Refusing to delete {profile!r} and its storage:\n{reasons}\n"
+            "Remove the profile by hand if you are certain."
         )
 
     if not yes and not click.confirm(f"Delete profile {profile!r} and its copy?"):

src/aiida_agents/sandbox/copy.py

@@ -36,14 +36,21 @@
 
 from __future__ import annotations
 
+import enum
 import shutil
 import typing as t
 import uuid
 from dataclasses import dataclass
 from pathlib import Path
 
+from typing_extensions import assert_never
+
 __all__ = [
+    "FILEPATH_BACKENDS",
+    "SUPPORTED_BACKENDS",
+    "Overlap",
     "SandboxStorage",
+    "SharingProfile",
     "profiles_sharing_storage",
     "register_profile",
     "copy_sqlite_storage",
@@ -52,13 +59,21 @@
     "sandbox_storage_root",
     "shares_storage",
     "storage_locations",
+    "storage_overlap",
 ]
 
-#: Backends this module knows how to copy. Anything else is refused by name
-#: rather than attempted, because a copy that half worked would be worse than
-#: no sandbox: it would look like containment.
+# Backends this module knows how to copy. Anything else is refused by name
+# rather than attempted, because a copy that half worked would be worse than
+# no sandbox: it would look like containment.
 SUPPORTED_BACKENDS = frozenset({"core.psql_dos", "core.sqlite_dos"})
 
+# Backends whose whole storage is the one path under `filepath`, and so can be
+# compared without loading them. Deliberately wider than SUPPORTED_BACKENDS: an
+# imported archive cannot be copied into a sandbox, but it can be told apart
+# from one, and until it could, every config holding an archive read as sharing
+# storage with the sandbox.
+FILEPATH_BACKENDS = frozenset({"core.sqlite_dos", "core.sqlite_zip"})
+
 
 @dataclass(frozen=True)
 class SandboxStorage:
@@ -80,18 +95,86 @@ def sandbox_storage_root(sandbox_name: str) -> Path:
     return Path(AiiDAConfigDir.get()) / "agents-sandbox" / sandbox_name
 
 
+class Overlap(enum.Enum):
+    """How one profile's storage compares with another's.
+
+    ``UNKNOWN`` is not a shade of ``SEPARATE``. It is what a backend or config
+    this module cannot read produces, it is acted on exactly like ``SHARED``,
+    and the two are kept apart only so the user can be told which one they
+    have: "these are the same directory" and "I have never heard of this
+    backend" call for different next steps.
+    """
+
+    SEPARATE = "separate"
+    SHARED = "shared"
+    UNKNOWN = "unknown"
+
+
+@dataclass(frozen=True)
+class SharingProfile:
+    """A profile that could not be proved separate from the one being checked.
+
+    ``overlap`` cannot be ``SEPARATE``: a profile proved separate is not one of
+    these at all.
+    """
+
+    name: str
+    backend: str
+    overlap: t.Literal[Overlap.SHARED, Overlap.UNKNOWN]
+
+    def describe(self) -> str:
+        """The reason, as a clause that follows the checked profile's name.
+
+        Here rather than at the call sites because ``sandbox check``,
+        ``sandbox teardown`` and ``doctor`` all report it, and three wordings
+        of one finding would drift.
+        """
+        if self.overlap is Overlap.SHARED:
+            return (
+                f"shares storage with {self.name!r}, so deleting either "
+                "destroys the other's data"
+            )
+        if self.overlap is Overlap.UNKNOWN:
+            return (
+                f"cannot be compared with {self.name!r}, which uses storage "
+                f"backend {self.backend!r} that this does not know how to read"
+            )
+        assert_never(self.overlap)  # pragma: no cover
+
+
+def _path_location(value: object) -> str | None:
+    """Canonical tag for a storage path, or ``None`` if it cannot be read as one.
+
+    The one place a configuration value becomes a comparable path: Postgres
+    writes its repository as a ``file://`` URL and the SQLite backends write a
+    plain ``filepath``, and both name a place on disk.
+
+    Relative paths are refused rather than resolved: :meth:`Path.resolve`
+    interprets them against the working directory, so the same two profiles
+    would compare differently depending on where the command was run.
+    """
+    if value is None:
+        return None
+    path = Path(str(value).removeprefix("file://"))
+    return f"path:{path.resolve()}" if path.is_absolute() else None
+
+
 def storage_locations(backend: str, config: dict[str, t.Any]) -> frozenset[str]:
     """Everything ``verdi profile delete --delete-data`` would destroy.
 
     Canonical strings rather than paths so that two profiles can be compared
-    without either being loadable. An empty set means "I could not tell", which
-    :func:`shares_storage` treats as sharing.
+    without either being loadable. Compared for equality, so a location *inside*
+    another one is not overlap as far as this is concerned. An empty set means
+    "I could not tell", which :func:`shares_storage` treats as sharing.
     """
-    if backend == "core.sqlite_dos":
-        filepath = config.get("filepath")
-        return (
-            frozenset({f"dir:{Path(filepath).resolve()}"}) if filepath else frozenset()
-        )
+    # core.sqlite_dos keeps its database and container in one directory;
+    # core.sqlite_zip is a single archive file, read-only in every respect but
+    # this one --- `SqliteZipBackend.delete` unlinks it, so `--delete-data`
+    # destroys it just the same. One tag for both, because two profiles naming
+    # one path share that path whatever each of them calls it.
+    if backend in FILEPATH_BACKENDS:
+        location = _path_location(config.get("filepath"))
+        return frozenset({location}) if location is not None else frozenset()
 
     if backend == "core.psql_dos":
         locations = set()
@@ -100,17 +183,34 @@ def storage_locations(backend: str, config: dict[str, t.Any]) -> frozenset[str]:
             host = config.get("database_hostname") or "localhost"
             port = config.get("database_port") or 5432
             locations.add(f"pg://{host}:{port}/{database}")
-        repository = config.get("repository_uri")
-        if repository:
-            locations.add(
-                f"dir:{Path(str(repository).removeprefix('file://')).resolve()}"
-            )
+        repository = _path_location(config.get("repository_uri"))
+        if repository is not None:
+            locations.add(repository)
         # A Postgres profile with neither is not something we can reason about.
         return frozenset(locations) if len(locations) == 2 else frozenset()
 
     return frozenset()
 
 
+def storage_overlap(
+    backend_a: str,
+    config_a: dict[str, t.Any],
+    backend_b: str,
+    config_b: dict[str, t.Any],
+) -> Overlap:
+    """How two profiles' storage compares, including *why* when it is not separate.
+
+    The comparison :func:`shares_storage` answers yes-or-no. Callers that
+    report the finding to a user need the distinction :class:`Overlap` draws,
+    and taking it from here keeps one implementation of the rule.
+    """
+    locations_a = storage_locations(backend_a, config_a)
+    locations_b = storage_locations(backend_b, config_b)
+    if not locations_a or not locations_b:
+        return Overlap.UNKNOWN
+    return Overlap.SHARED if locations_a & locations_b else Overlap.SEPARATE
+
+
 def shares_storage(
     backend_a: str,
     config_a: dict[str, t.Any],
@@ -128,14 +228,11 @@ def shares_storage(
     Sharing *either* the database or the repository counts. A sandbox with its
     own database but the real repository still loses the user their files.
     """
-    locations_a = storage_locations(backend_a, config_a)
-    locations_b = storage_locations(backend_b, config_b)
-    if not locations_a or not locations_b:
-        return True
-    return bool(locations_a & locations_b)
+    overlap = storage_overlap(backend_a, config_a, backend_b, config_b)
+    return overlap is not Overlap.SEPARATE
 
 
-def profiles_sharing_storage(config: t.Any, name: str) -> list[str]:
+def profiles_sharing_storage(config: t.Any, name: str) -> list[SharingProfile]:
     """Every other profile whose data would go with ``name``'s.
 
     The one implementation of this question. ``sandbox check`` asks it before
@@ -145,21 +242,29 @@ def profiles_sharing_storage(config: t.Any, name: str) -> list[str]:
     somebody deletes.
 
     Returns:
-        Profile names, empty when the sandbox is genuinely self-contained.
+        One entry per profile that could not be proved separate, carrying which
+        of the two reasons it was. Empty when the sandbox is genuinely
+        self-contained.
     """
     profiles = {profile.name: profile for profile in config.profiles}
     target = profiles[name]
-    return [
-        other.name
-        for other in config.profiles
-        if other.name != name
-        and shares_storage(
+    found = []
+    for other in config.profiles:
+        if other.name == name:
+            continue
+        overlap = storage_overlap(
             target.storage_backend,
             target.storage_config or {},
             other.storage_backend,
             other.storage_config or {},
         )
-    ]
+        if overlap is not Overlap.SEPARATE:
+            found.append(
+                SharingProfile(
+                    name=other.name, backend=other.storage_backend, overlap=overlap
+                )
+            )
+    return found
 
 
 def register_profile(config: t.Any, name: str, dictionary: dict[str, t.Any]) -> None:

tests/cli/test_commands.py

@@ -421,16 +421,66 @@ def test_check_fails_on_a_backend_it_cannot_reason_about(
         self, monkeypatch: pytest.MonkeyPatch
     ) -> None:
         """Fails closed. Not knowing whether two profiles are separate is not
-        the same as knowing they are, and only one of those mistakes is safe."""
+        the same as knowing they are, and only one of those mistakes is safe.
+
+        The stand-in has to be a backend that genuinely does not exist. This
+        test originally used `core.sqlite_zip`, which is a real built-in
+        backend for imported archives -- so it asserted that a perfectly
+        ordinary archive profile was unreadable, which is the bug #90 fixes
+        rather than the behaviour this test is for.
+        """
         config = self._config(
             self._profile("real", filepath="/data/real"),
-            self._profile("agents-sandbox", backend="core.sqlite_zip"),
+            self._profile("agents-sandbox", backend="thirdparty.custom_dos"),
+        )
+        self._patch(monkeypatch, config)
+
+        result = CliRunner().invoke(cli, ["sandbox", "check"])
+
+        assert result.exit_code == 1
+
+    def test_check_says_when_it_could_not_look_rather_than_what_it_found(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """A researcher reading "deleting this destroys their data" concludes
+        their sandbox is pointed at their own database. Where the truth is that
+        a backend could not be read, the message has to say so: the first is
+        theirs to fix, the second is not."""
+        config = self._config(
+            self._profile("odd", backend="thirdparty.custom_dos"),
+            self._profile("agents-sandbox", filepath="/data/copy"),
         )
         self._patch(monkeypatch, config)
 
         result = CliRunner().invoke(cli, ["sandbox", "check"])
+        output = " ".join(result.output.split())
 
         assert result.exit_code == 1
+        assert "cannot be compared with 'odd'" in output
+        assert "thirdparty.custom_dos" in output
+        assert "destroys" not in output
+
+    def test_check_passes_a_config_holding_an_archive_profile(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """An imported archive is a built-in backend, not an unreadable one.
+
+        Until `storage_locations` knew it, every config holding one made this
+        command name profiles the sandbox had never touched, and `teardown`
+        refused to remove the sandbox at all.
+        """
+        config = self._config(
+            self._profile("real", filepath="/data/real"),
+            self._profile(
+                "dev-archive", backend="core.sqlite_zip", filepath="/data/e.aiida"
+            ),
+            self._profile("agents-sandbox", filepath="/data/copy"),
+        )
+        self._patch(monkeypatch, config)
+
+        result = CliRunner().invoke(cli, ["sandbox", "check"])
+
+        assert result.exit_code == 0
 
     def test_init_refuses_a_backend_it_cannot_copy(
         self, monkeypatch: pytest.MonkeyPatch

tests/cli/test_doctor.py

@@ -281,3 +281,23 @@ def test_a_sandbox_sharing_storage_fails_the_check(
 
     assert row.ok is False
     assert "real" in row.detail
+
+
+def test_a_sharing_sandbox_is_not_sent_to_a_command_that_would_refuse(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """This row used to end in "rebuild with `aiida-agents sandbox refresh`".
+
+    Refresh tears the sandbox down before rebuilding it, and teardown refuses a
+    sandbox in exactly this state --- so the advice sent the reader to a command
+    that could not run, on the one row where they most need a next step that
+    works.
+    """
+    _patch_all_checks_passing(monkeypatch)
+    monkeypatch.setattr(
+        "aiida.manage.configuration.get_config",
+        lambda: _SandboxConfig("agents-sandbox", filepath="/data/real"),
+    )
+    row = _rows_by_label()["Codegen sandbox (disposable copy)"]
+
+    assert "refresh" not in row.detail