Add CatalogStorage protocol and SQLite implementation

tests/catalog/test_client_catalog.py

@@ -0,0 +1,128 @@
+"""Tests for Client catalog methods using CatalogClientMixin.
+
+These tests verify that the unified Client class can perform catalog operations
+via the CatalogClientMixin, spawning ephemeral workers for each call.
+
+IMPORTANT: The CatalogClientMixin spawns a NEW worker subprocess for each
+catalog operation. This means:
+- State doesn't persist between calls (each worker gets fresh InMemoryCatalog)
+- Tests requiring attach_id from a previous call will fail
+- Only stateless operations or single-call operations can be tested
+
+For full catalog functionality testing, use the direct InMemoryCatalog tests
+in tests/catalog/test_integration.py which test the catalog implementation
+directly without the subprocess boundary.
+"""
+
+from vgi.client import Client
+
+# Worker command for catalog tests
+CATALOG_WORKER = "vgi-example-catalog-worker"
+
+
+class TestClientCatalogStatelessOperations:
+    """Test catalog operations that don't require state persistence.
+
+    These tests work with the ephemeral worker pattern because they either:
+    - Don't require state from a previous call
+    - Complete in a single call
+
+    """
+
+    def test_catalogs_returns_list(self) -> None:
+        """Client.catalogs() returns list of catalog names."""
+        client = Client(CATALOG_WORKER)
+        catalogs = client.catalogs()
+        assert isinstance(catalogs, list)
+        assert "memory" in catalogs
+
+    def test_catalog_attach_returns_result(self) -> None:
+        """Client.catalog_attach() returns CatalogAttachResult."""
+        client = Client(CATALOG_WORKER)
+        result = client.catalog_attach(name="memory", options={})
+
+        assert result.attach_id is not None
+        assert len(result.attach_id) == 16  # UUID bytes
+        assert result.supports_transactions is False
+
+    def test_catalogs_works_without_start(self) -> None:
+        """Catalog methods work without calling start()."""
+        client = Client(CATALOG_WORKER)
+        # Don't call start() - catalog methods spawn ephemeral workers
+        catalogs = client.catalogs()
+        assert "memory" in catalogs
+
+    def test_catalogs_works_inside_context_manager(self) -> None:
+        """Catalog methods work inside context manager."""
+        with Client(CATALOG_WORKER) as client:
+            catalogs = client.catalogs()
+            assert "memory" in catalogs
+
+    def test_multiple_catalogs_calls(self) -> None:
+        """Multiple catalogs() calls work on same Client instance."""
+        client = Client(CATALOG_WORKER)
+
+        # Multiple calls should each spawn new workers and work independently
+        catalogs1 = client.catalogs()
+        catalogs2 = client.catalogs()
+
+        assert "memory" in catalogs1
+        assert "memory" in catalogs2
+
+    def test_catalog_attach_includes_capabilities(self) -> None:
+        """CatalogAttachResult includes capability flags."""
+        client = Client(CATALOG_WORKER)
+        result = client.catalog_attach(name="memory", options={})
+
+        # Check that capability flags are present (even if False)
+        assert isinstance(result.supports_transactions, bool)
+        assert isinstance(result.supports_time_travel, bool)
+        assert isinstance(result.catalog_version_frozen, bool)
+
+
+class TestClientCatalogProtocolIntegrity:
+    """Test that the catalog protocol is working correctly.
+
+    These tests verify the communication between Client and Worker without
+    requiring state persistence across calls.
+
+    """
+
+    def test_catalog_attach_different_attach_ids(self) -> None:
+        """Each catalog_attach call returns a different attach_id.
+
+        This verifies that the attach process is working, even though
+        the attach_id won't be usable in a subsequent call.
+
+        """
+        client = Client(CATALOG_WORKER)
+
+        # Each attach spawns a new worker, so each gets a unique ID
+        result1 = client.catalog_attach(name="memory", options={})
+        result2 = client.catalog_attach(name="memory", options={})
+
+        # Both should work, but will have different IDs
+        assert result1.attach_id is not None
+        assert result2.attach_id is not None
+        # IDs are randomly generated, so they're very likely different
+        # (not a guaranteed assertion, but useful for protocol verification)
+
+    def test_catalogs_returns_correct_format(self) -> None:
+        """catalogs() returns a list of strings."""
+        client = Client(CATALOG_WORKER)
+        catalogs = client.catalogs()
+
+        assert isinstance(catalogs, list)
+        for name in catalogs:
+            assert isinstance(name, str)
+
+
+# NOTE: Tests that require state persistence across catalog calls
+# (e.g., attach then use attach_id in subsequent call) are NOT possible
+# with the ephemeral worker pattern. Each call spawns a fresh worker
+# with a fresh InMemoryCatalog instance.
+#
+# To test full catalog workflows:
+# 1. Use tests/catalog/test_integration.py which tests InMemoryCatalog directly
+# 2. Or use a persistent catalog backend (e.g., SQLite-backed CatalogStorage)
+# 3. Or use a long-running worker process (not ephemeral)

tests/catalog/test_storage.py

@@ -0,0 +1,252 @@
+"""Tests for CatalogStorage and CatalogStorageSqlite."""
+
+import tempfile
+from pathlib import Path
+
+from vgi.catalog import AttachId, CatalogStorageSqlite, TransactionId
+
+
+class TestCatalogStorageSqliteAttachments:
+    """Test attachment operations."""
+
+    def test_attach_put_and_get(self) -> None:
+        """Can store and retrieve attachment state."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            attach_id = storage.generate_attach_id()
+            storage.attach_put(attach_id, "my_catalog", {"key": "value"})
+
+            result = storage.attach_get(attach_id)
+            assert result is not None
+            catalog_name, options = result
+            assert catalog_name == "my_catalog"
+            assert options == {"key": "value"}
+
+    def test_attach_get_nonexistent(self) -> None:
+        """Getting nonexistent attachment returns None."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            result = storage.attach_get(AttachId(b"nonexistent"))
+            assert result is None
+
+    def test_attach_delete(self) -> None:
+        """Can delete attachment state."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            attach_id = storage.generate_attach_id()
+            storage.attach_put(attach_id, "catalog", {})
+            storage.attach_delete(attach_id)
+
+            result = storage.attach_get(attach_id)
+            assert result is None
+
+    def test_attach_list(self) -> None:
+        """Can list all attachment IDs."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            id1 = storage.generate_attach_id()
+            id2 = storage.generate_attach_id()
+            storage.attach_put(id1, "catalog1", {})
+            storage.attach_put(id2, "catalog2", {})
+
+            ids = storage.attach_list()
+            assert len(ids) == 2
+            assert id1 in ids
+            assert id2 in ids
+
+    def test_attach_put_replaces_existing(self) -> None:
+        """Putting same attach_id replaces the existing entry."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            attach_id = storage.generate_attach_id()
+            storage.attach_put(attach_id, "old_catalog", {"old": True})
+            storage.attach_put(attach_id, "new_catalog", {"new": True})
+
+            result = storage.attach_get(attach_id)
+            assert result is not None
+            catalog_name, options = result
+            assert catalog_name == "new_catalog"
+            assert options == {"new": True}
+
+    def test_attach_with_complex_options(self) -> None:
+        """Can store and retrieve complex options."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            attach_id = storage.generate_attach_id()
+            options = {
+                "string": "value",
+                "number": 42,
+                "float": 3.14,
+                "bool": True,
+                "null": None,
+                "list": [1, 2, 3],
+                "nested": {"a": {"b": "c"}},
+            }
+            storage.attach_put(attach_id, "catalog", options)
+
+            result = storage.attach_get(attach_id)
+            assert result is not None
+            _, retrieved_options = result
+            assert retrieved_options == options
+
+
+class TestCatalogStorageSqliteTransactions:
+    """Test transaction operations."""
+
+    def test_transaction_put_and_get(self) -> None:
+        """Can store and retrieve transaction state."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            attach_id = storage.generate_attach_id()
+            storage.attach_put(attach_id, "catalog", {})
+
+            tx_id = storage.generate_transaction_id()
+            state = b"transaction state data"
+            storage.transaction_put(tx_id, attach_id, state)
+
+            result = storage.transaction_get(tx_id)
+            assert result is not None
+            retrieved_attach_id, retrieved_state = result
+            assert retrieved_attach_id == attach_id
+            assert retrieved_state == state
+
+    def test_transaction_get_nonexistent(self) -> None:
+        """Getting nonexistent transaction returns None."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            result = storage.transaction_get(TransactionId(b"nonexistent"))
+            assert result is None
+
+    def test_transaction_delete(self) -> None:
+        """Can delete transaction state."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            attach_id = storage.generate_attach_id()
+            storage.attach_put(attach_id, "catalog", {})
+
+            tx_id = storage.generate_transaction_id()
+            storage.transaction_put(tx_id, attach_id, b"state")
+            storage.transaction_delete(tx_id)
+
+            result = storage.transaction_get(tx_id)
+            assert result is None
+
+    def test_attach_delete_cascades_to_transactions(self) -> None:
+        """Deleting attachment also deletes associated transactions."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            attach_id = storage.generate_attach_id()
+            storage.attach_put(attach_id, "catalog", {})
+
+            tx_id = storage.generate_transaction_id()
+            storage.transaction_put(tx_id, attach_id, b"state")
+
+            # Delete the attachment
+            storage.attach_delete(attach_id)
+
+            # Transaction should also be deleted
+            result = storage.transaction_get(tx_id)
+            assert result is None
+
+
+class TestCatalogStorageSqliteIdGeneration:
+    """Test ID generation methods."""
+
+    def test_generate_attach_id_unique(self) -> None:
+        """Generated attach_ids are unique."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            ids = {storage.generate_attach_id() for _ in range(100)}
+            assert len(ids) == 100  # All unique
+
+    def test_generate_transaction_id_unique(self) -> None:
+        """Generated transaction_ids are unique."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            ids = {storage.generate_transaction_id() for _ in range(100)}
+            assert len(ids) == 100  # All unique
+
+    def test_attach_id_is_16_bytes(self) -> None:
+        """Generated attach_ids are 16 bytes (UUID)."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            attach_id = storage.generate_attach_id()
+            assert len(attach_id) == 16
+
+
+class TestCatalogStorageSqliteCleanup:
+    """Test cleanup operations."""
+
+    def test_cleanup_returns_count(self) -> None:
+        """Cleanup returns the count of deleted entries."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            # With no entries, should return 0
+            deleted = storage.cleanup_old_entries(max_age_days=0.0)
+            assert deleted == 0
+
+    def test_cleanup_preserves_recent_entries(self) -> None:
+        """Cleanup with large max_age preserves recent entries."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+            storage = CatalogStorageSqlite(db_path)
+
+            attach_id = storage.generate_attach_id()
+            storage.attach_put(attach_id, "catalog", {})
+
+            # Cleanup with 365 days should not remove recent entry
+            storage.cleanup_old_entries(max_age_days=365.0)
+
+            result = storage.attach_get(attach_id)
+            assert result is not None
+
+
+class TestCatalogStorageSqlitePersistence:
+    """Test persistence across storage instances."""
+
+    def test_persistence_across_instances(self) -> None:
+        """Data persists across storage instances."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = str(Path(tmpdir) / "test.db")
+
+            # Create and store with first instance
+            storage1 = CatalogStorageSqlite(db_path)
+            attach_id = storage1.generate_attach_id()
+            storage1.attach_put(attach_id, "persistent_catalog", {"key": "value"})
+
+            # Create second instance and retrieve
+            storage2 = CatalogStorageSqlite(db_path)
+            result = storage2.attach_get(attach_id)
+
+            assert result is not None
+            catalog_name, options = result
+            assert catalog_name == "persistent_catalog"
+            assert options == {"key": "value"}

tests/client/test_cli.py

@@ -259,11 +259,13 @@ def test_invalid_attach_id_hex(self, example_worker: str) -> None:
         assert result.exit_code != 0
         assert "valid hex string" in result.output
 
-    def test_missing_required_function(self) -> None:
-        """--function is required."""
+    def test_missing_function_shows_help(self) -> None:
+        """Calling CLI with no arguments shows help (group behavior)."""
         runner = CliRunner()
         result = runner.invoke(cli, [])
-        assert result.exit_code != 0
+        # With Click group, no subcommand and no --function shows help
+        assert result.exit_code == 0
+        assert "Usage:" in result.output
 
 
 class TestCLITableFunction: