feat: Phase 3 — ServiceComposer deps + store install via per-service compose

ServiceStoreManager.install() now delegates container lifecycle to
ServiceComposer (per-service docker-compose.yml) instead of appending to a
shared compose override. This eliminates IP pool allocation, compose override
rendering, and the single-stack docker exec approach.

Changes:
- service_composer.py: add _resolve_requires(), _resolve_dependents(),
  reapply_active_services() — dependency graph and startup reapply
- service_store_manager.py: rewrite install() and remove() to use
  ServiceComposer; add _fetch_template(); delete _allocate_service_ip(),
  _render_compose_override(), _write_compose_override(); remove() now guards
  against removing services that others depend on
- managers.py: pass service_composer= to ServiceStoreManager
- Tests: 13 new composer dep tests; TestInstall/TestRemove rewritten for
  the new composer-driven path; test_optional_services_feature.py updated

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

api/managers.py

@@ -93,6 +93,7 @@ service_store_manager = ServiceStoreManager(
     container_manager=container_manager,
     data_dir=DATA_DIR,
     config_dir=CONFIG_DIR,
+    service_composer=service_composer,
 )
 
 # Service logger configuration

api/service_composer.py

@@ -277,6 +277,39 @@ class ServiceComposer:
                 pass
         return result
 
+    # ── Dependency resolution ─────────────────────────────────────────────
+
+    def _resolve_requires(self, manifest: Dict, installed_services: Dict) -> Optional[str]:
+        """Return an error string if any required services are missing, else None."""
+        requires = manifest.get('requires') or []
+        missing = [r for r in requires if r not in installed_services]
+        if missing:
+            return f"Required services not installed: {', '.join(sorted(missing))}"
+        return None
+
+    def _resolve_dependents(self, service_id: str, installed_services: Dict) -> List[str]:
+        """Return list of installed service IDs that declare service_id in their requires."""
+        dependents = []
+        for svc_id, record in installed_services.items():
+            if svc_id == service_id:
+                continue
+            m = (record.get('manifest') or {})
+            if service_id in (m.get('requires') or []):
+                dependents.append(svc_id)
+        return dependents
+
+    def reapply_active_services(self) -> None:
+        """Call up() for every installed service that has a compose file. Called at startup."""
+        installed = self.cm.get_installed_services()
+        for svc_id in installed:
+            if not self.has_compose_file(svc_id):
+                logger.warning('reapply_active_services: no compose file for %s, skipping', svc_id)
+                continue
+            result = self.up(svc_id)
+            if not result.get('ok'):
+                logger.warning('reapply_active_services: up failed for %s: %s',
+                               svc_id, result.get('error') or result.get('stderr', ''))
+
     # ── Builtin-service lifecycle (main compose stack) ─────────────────────
 
     @staticmethod

api/service_store_manager.py

@@ -14,17 +14,14 @@ import logging
 import os
 import re
 import threading
-import subprocess
 from datetime import datetime
 from typing import Any, Dict, List, Optional, Tuple
 
 import json
 
 import requests
-import yaml
 
 from base_service_manager import BaseServiceManager
-from ip_utils import CONTAINER_OFFSETS
 from manifest_validator import validate_manifest, validate_provision_hook
 
 logger = logging.getLogger(__name__)
@@ -33,15 +30,15 @@ logger = logging.getLogger(__name__)
 # Constants
 # ---------------------------------------------------------------------------
 
-SERVICE_POOL_START = 20
-SERVICE_POOL_END = 254
-
 INDEX_URL_DEFAULT = (
     'https://git.pic.ngo/roof/pic-services/raw/branch/main/index.json'
 )
 MANIFEST_URL_TPL = (
     'https://git.pic.ngo/roof/pic-services/raw/branch/main/services/{id}/manifest.json'
 )
+TEMPLATE_URL_TPL = (
+    'https://git.pic.ngo/roof/pic-services/raw/branch/main/services/{id}/compose-template.yml'
+)
 
 IMAGE_ALLOWLIST_RE = re.compile(
     r'^git\.pic\.ngo/roof/[a-z0-9._/-]+(:[a-zA-Z0-9._-]+)?(@sha256:[a-f0-9]{64})?$'
@@ -77,11 +74,13 @@ class ServiceStoreManager(BaseServiceManager):
     """Manages service store: install, remove, and list available/installed services."""
 
     def __init__(self, config_manager, caddy_manager, container_manager,
-                 data_dir: str = '', config_dir: str = ''):
+                 data_dir: str = '', config_dir: str = '',
+                 service_composer=None):
         super().__init__('service_store', data_dir, config_dir)
         self.config_manager = config_manager
         self.caddy_manager = caddy_manager
         self.container_manager = container_manager
+        self.service_composer = service_composer
         self.compose_override = os.environ.get(
             'COMPOSE_SERVICES_PATH', '/app/docker-compose.services.yml'
         )
@@ -239,125 +238,6 @@ class ServiceStoreManager(BaseServiceManager):
 
         return (len(errors) == 0, errors)
 
-    # ── IP allocation ─────────────────────────────────────────────────────
-
-    def _allocate_service_ip(self, service_id: str) -> str:
-        """Allocate the next free IP from the service pool."""
-        identity = self.config_manager.get_identity()
-        ip_range = identity.get('ip_range', '172.20.0.0/16')
-
-        import ipaddress
-        network = ipaddress.IPv4Network(ip_range, strict=False)
-        base = int(network.network_address)
-
-        # IPs already assigned to named containers
-        reserved_offsets = set(CONTAINER_OFFSETS.values())
-
-        # IPs already assigned to installed services
-        service_ips: Dict[str, str] = identity.get('service_ips', {})
-        taken_ips = set(service_ips.values())
-
-        for offset in range(SERVICE_POOL_START, SERVICE_POOL_END + 1):
-            if offset in reserved_offsets:
-                continue
-            candidate = str(ipaddress.IPv4Address(base + offset))
-            if candidate not in taken_ips:
-                return candidate
-
-        raise RuntimeError('Service IP pool exhausted (offsets 20-254 all taken)')
-
-    # ── Compose override ──────────────────────────────────────────────────
-
-    def _render_compose_override(self, installed_records: dict) -> str:
-        """Generate docker-compose YAML override for all installed services."""
-        services: Dict[str, Any] = {}
-
-        for svc_id, record in installed_records.items():
-            manifest = record.get('manifest', {})
-            container_name = record.get('container_name', svc_id)
-            image = manifest.get('image', record.get('image', ''))
-            service_ip = record.get('service_ip', '')
-
-            # Volumes
-            volumes = []
-            for vol in manifest.get('volumes', []):
-                vol_name = vol.get('name', '')
-                mount = vol.get('mount', '')
-                if vol_name and mount:
-                    volumes.append(f'{vol_name}:{mount}')
-
-            # Environment
-            environment: Dict[str, str] = {}
-            for env_entry in manifest.get('env', []):
-                k = env_entry.get('key', '')
-                v = str(env_entry.get('value', ''))
-                if k:
-                    environment[k] = v
-
-            svc_def: Dict[str, Any] = {
-                'image': image,
-                'container_name': container_name,
-                'restart': 'unless-stopped',
-                'logging': {
-                    'driver': 'json-file',
-                    'options': {
-                        'max-size': '10m',
-                        'max-file': '5',
-                    },
-                },
-                'networks': {
-                    'cell-network': {
-                        'ipv4_address': service_ip,
-                    }
-                },
-            }
-            if volumes:
-                svc_def['volumes'] = volumes
-            if environment:
-                svc_def['environment'] = environment
-
-            services[container_name] = svc_def
-
-        # Collect named volumes
-        named_volumes: Dict[str, Any] = {}
-        for svc_id, record in installed_records.items():
-            manifest = record.get('manifest', {})
-            for vol in manifest.get('volumes', []):
-                vol_name = vol.get('name', '')
-                if vol_name:
-                    named_volumes[vol_name] = None  # Docker default driver
-
-        doc: Dict[str, Any] = {
-            'version': '3.8',
-            'services': services,
-            'networks': {
-                'cell-network': {
-                    'external': True,
-                }
-            },
-        }
-        if named_volumes:
-            doc['volumes'] = named_volumes
-
-        return yaml.dump(doc, default_flow_style=False, allow_unicode=True)
-
-    def _write_compose_override(self, content: str) -> None:
-        """Atomic write of the compose override file."""
-        tmp_path = self.compose_override + '.tmp'
-        try:
-            os.makedirs(os.path.dirname(os.path.abspath(self.compose_override)),
-                        exist_ok=True)
-        except (PermissionError, OSError):
-            pass
-        with open(tmp_path, 'w') as f:
-            f.write(content)
-            f.flush()
-            try:
-                os.fsync(f.fileno())
-            except OSError:
-                pass
-        os.replace(tmp_path, self.compose_override)
-
     # ── Index / manifest fetching ─────────────────────────────────────────
 
     def fetch_index(self) -> list:
@@ -394,14 +274,22 @@ class ServiceStoreManager(BaseServiceManager):
             )
         return json.loads(content)
 
+    def _fetch_template(self, service_id: str, manifest: dict) -> str:
+        """Fetch the compose template for a service."""
+        _SIZE_LIMIT = 256 * 1024
+        url = TEMPLATE_URL_TPL.format(id=service_id)
+        resp = requests.get(url, timeout=10, stream=True)
+        resp.raise_for_status()
+        content = resp.raw.read(_SIZE_LIMIT + 1, decode_content=True)
+        if len(content) > _SIZE_LIMIT:
+            raise ValueError(f'Compose template for {service_id} exceeds 256 KB limit')
+        return content.decode('utf-8')
+
     # ── Core operations ───────────────────────────────────────────────────
 
     def install(self, service_id: str) -> dict:
         """Install a service from the store."""
-        from firewall_manager import apply_service_rules
-
         with self._lock:
-            # Already installed?
             installed = self.config_manager.get_installed_services()
             if service_id in installed:
                 return {'ok': True, 'already_installed': True}
@@ -416,154 +304,80 @@ class ServiceStoreManager(BaseServiceManager):
             if not ok:
                 return {'ok': False, 'errors': errs}
 
-            # Allocate IP
+            ok2, errs2 = validate_manifest(manifest)
-            try:
+            if not ok2:
-                ip = self._allocate_service_ip(service_id)
+                return {'ok': False, 'errors': errs2}
-            except RuntimeError as e:
-                return {'ok': False, 'error': str(e)}
 
-            # Build install record
+            # Dependency check
+            if self.service_composer is not None:
+                err = self.service_composer._resolve_requires(manifest, installed)
+                if err:
+                    return {'ok': False, 'error': err}
+
+            # Fetch compose template
+            try:
+                template_content = self._fetch_template(service_id, manifest)
+            except Exception as e:
+                return {'ok': False, 'error': f'Failed to fetch compose template: {e}'}
+
+            # Write compose file and start containers (validation inside write_compose)
+            if self.service_composer is not None:
+                try:
+                    result = self.service_composer.install(service_id, manifest, template_content)
+                except ValueError as e:
+                    return {'ok': False, 'error': str(e)}
+                except Exception as e:
+                    return {'ok': False, 'error': f'Failed to start service: {e}'}
+                if not result.get('ok'):
+                    return {'ok': False, 'error': result.get('error') or result.get('stderr', 'docker up failed')}
+
+            # Persist minimal install record
             record = {
                 'id': service_id,
-                'name': manifest.get('name', service_id),
-                'container_name': manifest['container_name'],
-                'image': manifest.get('image', ''),
-                'service_ip': ip,
-                'caddy_route': manifest.get('caddy_route'),
-                'iptables_rules': manifest.get('iptables_rules', []),
                 'manifest': manifest,
                 'installed_at': datetime.utcnow().isoformat(),
             }
-
-            # Persist to config
             self.config_manager.set_installed_service(service_id, record)
-            identity = self.config_manager.get_identity()
-            service_ips = dict(identity.get('service_ips', {}))
-            service_ips[service_id] = ip
-            self.config_manager.set_identity_field('service_ips', service_ips)
 
-            # Write compose override
+            # Regenerate Caddy (registry now drives routes, no caddy_routes list needed)
-            all_installed = self.config_manager.get_installed_services()
             try:
-                content = self._render_compose_override(all_installed)
+                self.caddy_manager.regenerate_with_installed([])
-                self._write_compose_override(content)
             except Exception as e:
-                logger.error(f'Failed to write compose override: {e}')
+                logger.warning('install: caddy regenerate failed for %s (non-fatal): %s', service_id, e)
 
-            # Apply iptables rules (best-effort)
+            return {'ok': True}
-            try:
-                apply_service_rules(service_id, ip, manifest.get('iptables_rules', []))
-            except Exception as e:
-                logger.warning(f'apply_service_rules for {service_id} failed (non-fatal): {e}')
-
-            # Regenerate Caddyfile
-            try:
-                caddy_routes = [
-                    r.get('caddy_route')
-                    for r in all_installed.values()
-                    if r.get('caddy_route')
-                ]
-                self.caddy_manager.regenerate_with_installed(caddy_routes)
-            except Exception as e:
-                logger.warning(f'caddy regenerate for {service_id} failed (non-fatal): {e}')
-
-            # Start the container via docker compose
-            base_compose = os.environ.get('COMPOSE_FILE', '/app/docker-compose.yml')
-            try:
-                result = subprocess.run(
-                    ['docker', 'compose',
-                     '-f', base_compose,
-                     '-f', self.compose_override,
-                     'up', '-d', manifest['container_name']],
-                    capture_output=True, text=True, timeout=120,
-                )
-                if result.returncode != 0:
-                    logger.warning(
-                        f'docker compose up for {service_id} failed: {result.stderr.strip()}'
-                    )
-            except Exception as e:
-                logger.warning(f'docker compose up for {service_id} failed (non-fatal): {e}')
-
-            return {
-                'ok': True,
-                'service_ip': ip,
-                'container_name': manifest['container_name'],
-            }
 
     def remove(self, service_id: str, purge_data: bool = False) -> dict:
         """Remove an installed service."""
-        from firewall_manager import clear_service_rules
-
         with self._lock:
             installed = self.config_manager.get_installed_services()
-            record = installed.get(service_id)
+            if service_id not in installed:
-            if not record:
                 return {'ok': False, 'error': f'Service {service_id} is not installed'}
 
-            container_name = record.get('container_name', service_id)
+            # Prevent removing a service that others depend on
-            manifest = record.get('manifest', {})
+            if self.service_composer is not None:
-            base_compose = os.environ.get('COMPOSE_FILE', '/app/docker-compose.yml')
+                dependents = self.service_composer._resolve_dependents(service_id, installed)
+                if dependents:
+                    return {
+                        'ok': False,
+                        'error': f'Cannot remove {service_id}: required by {", ".join(sorted(dependents))}',
+                    }
 
-            # Stop and remove container
+            # Stop and remove containers (best-effort)
-            try:
+            if self.service_composer is not None:
-                subprocess.run(
+                try:
-                    ['docker', 'compose',
+                    self.service_composer.remove(service_id, purge_data=purge_data)
-                     '-f', base_compose,
+                except Exception as e:
-                     '-f', self.compose_override,
+                    logger.warning('remove: composer.remove failed for %s (non-fatal): %s', service_id, e)
-                     'stop', container_name],
-                    capture_output=True, text=True, timeout=60,
-                )
-            except Exception as e:
-                logger.warning(f'docker compose stop for {service_id} failed (non-fatal): {e}')
 
-            try:
+            # Remove from config
-                subprocess.run(
-                    ['docker', 'rm', '-f', container_name],
-                    capture_output=True, text=True, timeout=30,
-                )
-            except Exception as e:
-                logger.warning(f'docker rm for {service_id} failed (non-fatal): {e}')
-
-            # Clear iptables rules
-            try:
-                clear_service_rules(service_id)
-            except Exception as e:
-                logger.warning(f'clear_service_rules for {service_id} failed (non-fatal): {e}')
-
-            # Remove from config, regenerate compose + caddy
             self.config_manager.remove_installed_service(service_id)
-            remaining = self.config_manager.get_installed_services()
 
+            # Regenerate Caddy
             try:
-                content = self._render_compose_override(remaining)
+                self.caddy_manager.regenerate_with_installed([])
-                self._write_compose_override(content)
             except Exception as e:
-                logger.error(f'Failed to write compose override after remove: {e}')
+                logger.warning('remove: caddy regenerate failed for %s (non-fatal): %s', service_id, e)
-
-            try:
-                caddy_routes = [
-                    r.get('caddy_route')
-                    for r in remaining.values()
-                    if r.get('caddy_route')
-                ]
-                self.caddy_manager.regenerate_with_installed(caddy_routes)
-            except Exception as e:
-                logger.warning(f'caddy regenerate after remove failed (non-fatal): {e}')
-
-            # Purge named volumes if requested
-            if purge_data:
-                for vol in manifest.get('volumes', []):
-                    vol_name = vol.get('name', '')
-                    if vol_name:
-                        try:
-                            subprocess.run(
-                                ['docker', 'volume', 'rm', vol_name],
-                                capture_output=True, text=True, timeout=30,
-                            )
-                        except Exception as e:
-                            logger.warning(
-                                f'docker volume rm {vol_name} failed (non-fatal): {e}'
-                            )
 
             return {'ok': True}
 
@@ -581,13 +395,6 @@ class ServiceStoreManager(BaseServiceManager):
         if not installed:
             return
 
-        # Regenerate compose override in case it was deleted
-        try:
-            content = self._render_compose_override(installed)
-            self._write_compose_override(content)
-        except Exception as e:
-            logger.warning(f'reapply_on_startup: compose override write failed: {e}')
-
         # Re-apply iptables rules
         for svc_id, record in installed.items():
             ip = record.get('service_ip', '')
@@ -607,3 +414,10 @@ class ServiceStoreManager(BaseServiceManager):
             self.caddy_manager.regenerate_with_installed(caddy_routes)
         except Exception as e:
             logger.warning(f'reapply_on_startup: caddy regenerate failed: {e}')
+
+        # Bring up per-service compose stacks
+        if self.service_composer is not None:
+            try:
+                self.service_composer.reapply_active_services()
+            except Exception as e:
+                logger.warning('reapply_on_startup: reapply_active_services failed: %s', e)

tests/test_optional_services_feature.py

@@ -299,64 +299,63 @@ def _make_ssm(tmp_dir, installed=None, identity=None):
     }
     caddy = MagicMock()
     container = MagicMock()
+    composer = MagicMock()
+    composer._resolve_requires.return_value = None
+    composer._resolve_dependents.return_value = []
+    composer.install.return_value = {'ok': True}
+    composer.remove.return_value = {'ok': True}
     mgr = ServiceStoreManager(
         config_manager=cm,
         caddy_manager=caddy,
         container_manager=container,
         data_dir=tmp_dir,
         config_dir=tmp_dir,
+        service_composer=composer,
     )
-    mgr.compose_override = os.path.join(tmp_dir, 'docker-compose.services.yml')
     return mgr
 
 
 class TestInstallHappyPath(unittest.TestCase):
 
     def test_install_fetches_manifest_renders_compose_calls_docker_up(self):
-        """install() happy path: fetches manifest, writes compose, calls docker compose up."""
+        """install() happy path: fetches manifest, calls service_composer.install, stores record."""
         with tempfile.TemporaryDirectory() as tmp:
             mgr = _make_ssm(tmp)
             manifest = _ssm_manifest('email')
             mgr._fetch_manifest = MagicMock(return_value=manifest)
-            mgr._write_compose_override = MagicMock()
+            mgr._fetch_template = MagicMock(return_value='version: "3"\nservices: {}\n')
 
-            with patch('firewall_manager.apply_service_rules'), \
+            result = mgr.install('email')
-                 patch('service_store_manager.subprocess.run') as mock_run:
-                mock_run.return_value = MagicMock(returncode=0, stderr='')
-                result = mgr.install('email')
 
             self.assertTrue(result['ok'])
             mgr._fetch_manifest.assert_called_once_with('email')
             mgr.config_manager.set_installed_service.assert_called_once()
-            # docker compose up must have been called
+            # service_composer.install must have been called
-            self.assertTrue(mock_run.called)
+            mgr.service_composer.install.assert_called_once()
-            docker_cmd = mock_run.call_args[0][0]
-            self.assertIn('up', docker_cmd)
-            self.assertIn('-d', docker_cmd)
 
-    def test_install_persists_install_record_before_docker_up(self):
+    def test_install_persists_install_record_after_composer_install(self):
-        """Install record must be written to config before docker compose up is attempted."""
+        """Install record must be written after service_composer.install succeeds."""
         call_order = []
         with tempfile.TemporaryDirectory() as tmp:
             mgr = _make_ssm(tmp)
             manifest = _ssm_manifest('calendar')
             mgr._fetch_manifest = MagicMock(return_value=manifest)
-            mgr._write_compose_override = MagicMock()
+            mgr._fetch_template = MagicMock(return_value='version: "3"\nservices: {}\n')
             mgr.config_manager.set_installed_service.side_effect = \
                 lambda *a, **kw: call_order.append('set_installed')
 
-            with patch('firewall_manager.apply_service_rules'), \
+            def _composer_install(*a, **kw):
-                 patch('service_store_manager.subprocess.run') as mock_run:
+                call_order.append('composer_install')
-                def _docker(*a, **kw):
+                return {'ok': True}
-                    call_order.append('docker_up')
+            mgr.service_composer.install.side_effect = _composer_install
-                    return MagicMock(returncode=0, stderr='')
+            mgr.install('calendar')
-                mock_run.side_effect = _docker
-                mgr.install('calendar')
 
+        self.assertIn('composer_install', call_order)
+        self.assertIn('set_installed', call_order)
         self.assertLess(
+            call_order.index('composer_install'),
             call_order.index('set_installed'),
-            call_order.index('docker_up'),
+            'composer.install must be called before install record is persisted',
-            'install record must be written before docker compose up',
         )
 
 
@@ -367,8 +366,7 @@ class TestInstallAlreadyInstalled(unittest.TestCase):
         with tempfile.TemporaryDirectory() as tmp:
             installed = {'email': {'id': 'email'}}
             mgr = _make_ssm(tmp, installed=installed)
-            with patch('firewall_manager.apply_service_rules'):
+            result = mgr.install('email')
-                result = mgr.install('email')
         self.assertTrue(result['ok'])
         self.assertTrue(result.get('already_installed'))
 
@@ -378,8 +376,7 @@ class TestInstallAlreadyInstalled(unittest.TestCase):
             installed = {'email': {'id': 'email'}}
             mgr = _make_ssm(tmp, installed=installed)
             mgr._fetch_manifest = MagicMock()
-            with patch('firewall_manager.apply_service_rules'):
+            mgr.install('email')
-                mgr.install('email')
         mgr._fetch_manifest.assert_not_called()
 
     def test_install_already_installed_does_not_write_config(self):
@@ -387,8 +384,7 @@ class TestInstallAlreadyInstalled(unittest.TestCase):
         with tempfile.TemporaryDirectory() as tmp:
             installed = {'calendar': {'id': 'calendar'}}
             mgr = _make_ssm(tmp, installed=installed)
-            with patch('firewall_manager.apply_service_rules'):
+            mgr.install('calendar')
-                mgr.install('calendar')
         mgr.config_manager.set_installed_service.assert_not_called()
 
 
@@ -427,47 +423,6 @@ class TestInstallManifestFetchFails(unittest.TestCase):
         self.assertFalse(result['ok'])
         mgr.config_manager.set_installed_service.assert_not_called()
 
-
-class TestInstallComposeUpFails(unittest.TestCase):
-    """
-    The current implementation writes the install record BEFORE docker compose up.
-    When compose up fails the install record is already written — that is the
-    existing (accepted) behaviour documented in the implementation.
-
-    These tests verify the error is surfaced correctly rather than silently swallowed,
-    and that the install record IS present (not rolled back) after a compose failure.
-    """
-
-    def test_install_compose_failure_is_logged_not_raised(self):
-        """A non-zero exit from docker compose up must not raise — it is logged."""
-        with tempfile.TemporaryDirectory() as tmp:
-            mgr = _make_ssm(tmp)
-            manifest = _ssm_manifest('email')
-            mgr._fetch_manifest = MagicMock(return_value=manifest)
-            mgr._write_compose_override = MagicMock()
-            with patch('firewall_manager.apply_service_rules'), \
-                 patch('service_store_manager.subprocess.run') as mock_run:
-                mock_run.return_value = MagicMock(
-                    returncode=1, stderr='image pull failed'
-                )
-                # Must not raise
-                result = mgr.install('email')
-        # ok is still True because the record was persisted (compose is best-effort)
-        self.assertTrue(result['ok'])
-
-    def test_install_record_written_even_when_compose_fails(self):
-        """Install record must exist after compose failure (compose is best-effort)."""
-        with tempfile.TemporaryDirectory() as tmp:
-            mgr = _make_ssm(tmp)
-            manifest = _ssm_manifest('email')
-            mgr._fetch_manifest = MagicMock(return_value=manifest)
-            mgr._write_compose_override = MagicMock()
-            with patch('firewall_manager.apply_service_rules'), \
-                 patch('service_store_manager.subprocess.run') as mock_run:
-                mock_run.return_value = MagicMock(returncode=1, stderr='pull failed')
-                mgr.install('email')
-        mgr.config_manager.set_installed_service.assert_called_once()
-
     def test_install_invalid_manifest_does_not_write_record(self):
         """Manifest validation failure must prevent any install record from being written."""
         with tempfile.TemporaryDirectory() as tmp:
@@ -484,6 +439,36 @@ class TestInstallComposeUpFails(unittest.TestCase):
         mgr.config_manager.set_installed_service.assert_not_called()
 
 
+class TestInstallComposeUpFails(unittest.TestCase):
+    """
+    In the new architecture, a compose failure from service_composer.install returns
+    ok=False immediately — the install record is NOT written when compose fails.
+    """
+
+    def test_install_compose_failure_returns_error(self):
+        """A failure from service_composer.install must return ok=False."""
+        with tempfile.TemporaryDirectory() as tmp:
+            mgr = _make_ssm(tmp)
+            manifest = _ssm_manifest('email')
+            mgr._fetch_manifest = MagicMock(return_value=manifest)
+            mgr._fetch_template = MagicMock(return_value='version: "3"\nservices: {}\n')
+            mgr.service_composer.install.return_value = {'ok': False, 'stderr': 'image pull failed'}
+            result = mgr.install('email')
+        self.assertFalse(result['ok'])
+        self.assertIn('error', result)
+
+    def test_install_record_not_written_when_compose_fails(self):
+        """Install record must NOT be written when service_composer.install fails."""
+        with tempfile.TemporaryDirectory() as tmp:
+            mgr = _make_ssm(tmp)
+            manifest = _ssm_manifest('email')
+            mgr._fetch_manifest = MagicMock(return_value=manifest)
+            mgr._fetch_template = MagicMock(return_value='version: "3"\nservices: {}\n')
+            mgr.service_composer.install.return_value = {'ok': False, 'stderr': 'pull failed'}
+            mgr.install('email')
+        mgr.config_manager.set_installed_service.assert_not_called()
+
+
 # ---------------------------------------------------------------------------
 # 6. ServiceStoreManager.uninstall() (remove())
 # ---------------------------------------------------------------------------
@@ -492,62 +477,39 @@ class TestUninstallHappyPath(unittest.TestCase):
 
     def _make_mgr_with_email(self, tmp):
         record = {
-            'container_name': 'cell-email',
+            'id': 'email',
-            'service_ip': '172.20.0.20',
             'manifest': {
                 'image': 'git.pic.ngo/roof/email:1.0',
-                'volumes': [],
             },
-            'iptables_rules': [],
         }
         installed = {'email': record}
         mgr = _make_ssm(tmp, installed=installed)
         mgr.config_manager.remove_installed_service = MagicMock()
-        mgr.config_manager.get_installed_services.side_effect = [
-            installed,   # first call: existence check
-            {},          # second call: after removal, compose rewrite
-        ]
-        mgr._write_compose_override = MagicMock()
         return mgr
 
     def test_uninstall_happy_path_returns_ok_true(self):
         with tempfile.TemporaryDirectory() as tmp:
             mgr = self._make_mgr_with_email(tmp)
-            with patch('firewall_manager.clear_service_rules'), \
+            result = mgr.remove('email')
-                 patch('service_store_manager.subprocess.run') as mock_run:
-                mock_run.return_value = MagicMock(returncode=0, stderr='')
-                result = mgr.remove('email')
         self.assertTrue(result['ok'])
 
     def test_uninstall_removes_install_record(self):
         with tempfile.TemporaryDirectory() as tmp:
             mgr = self._make_mgr_with_email(tmp)
-            with patch('firewall_manager.clear_service_rules'), \
+            mgr.remove('email')
-                 patch('service_store_manager.subprocess.run') as mock_run:
-                mock_run.return_value = MagicMock(returncode=0, stderr='')
-                mgr.remove('email')
         mgr.config_manager.remove_installed_service.assert_called_once_with('email')
 
-    def test_uninstall_calls_docker_compose_stop_and_rm(self):
+    def test_uninstall_calls_service_composer_remove(self):
+        """New architecture: composer.remove() is called instead of subprocess directly."""
         with tempfile.TemporaryDirectory() as tmp:
             mgr = self._make_mgr_with_email(tmp)
-            with patch('firewall_manager.clear_service_rules'), \
+            mgr.remove('email')
-                 patch('service_store_manager.subprocess.run') as mock_run:
+        mgr.service_composer.remove.assert_called_once_with('email', purge_data=False)
-                mock_run.return_value = MagicMock(returncode=0, stderr='')
-                mgr.remove('email')
-        calls_str = [str(c) for c in mock_run.call_args_list]
-        has_stop = any('stop' in c for c in calls_str)
-        has_rm = any('rm' in c for c in calls_str)
-        self.assertTrue(has_stop, 'docker compose stop should have been called')
-        self.assertTrue(has_rm, 'docker rm should have been called')
 
     def test_uninstall_regenerates_caddyfile(self):
         with tempfile.TemporaryDirectory() as tmp:
             mgr = self._make_mgr_with_email(tmp)
-            with patch('firewall_manager.clear_service_rules'), \
+            mgr.remove('email')
-                 patch('service_store_manager.subprocess.run') as mock_run:
-                mock_run.return_value = MagicMock(returncode=0, stderr='')
-                mgr.remove('email')
         mgr.caddy_manager.regenerate_with_installed.assert_called()
 
 
@@ -556,26 +518,22 @@ class TestUninstallNotInstalled(unittest.TestCase):
     def test_uninstall_service_not_installed_returns_error(self):
         with tempfile.TemporaryDirectory() as tmp:
             mgr = _make_ssm(tmp, installed={})
-            with patch('firewall_manager.clear_service_rules'):
+            result = mgr.remove('email')
-                result = mgr.remove('email')
         self.assertFalse(result['ok'])
         self.assertIn('error', result)
         self.assertIn('not installed', result['error'].lower())
 
-    def test_uninstall_nonexistent_service_does_not_call_docker(self):
+    def test_uninstall_nonexistent_service_does_not_call_composer(self):
         with tempfile.TemporaryDirectory() as tmp:
             mgr = _make_ssm(tmp, installed={})
-            with patch('firewall_manager.clear_service_rules'), \
+            mgr.remove('email')
-                 patch('service_store_manager.subprocess.run') as mock_run:
+        mgr.service_composer.remove.assert_not_called()
-                mgr.remove('email')
-        mock_run.assert_not_called()
 
     def test_uninstall_nonexistent_service_does_not_remove_config(self):
         with tempfile.TemporaryDirectory() as tmp:
             mgr = _make_ssm(tmp, installed={})
             mgr.config_manager.remove_installed_service = MagicMock()
-            with patch('firewall_manager.clear_service_rules'):
+            mgr.remove('email')
-                mgr.remove('email')
         mgr.config_manager.remove_installed_service.assert_not_called()
 
 

tests/test_service_composer.py

@@ -532,5 +532,109 @@ class TestParsePsJson(unittest.TestCase):
         self.assertEqual(len(result), 1)
 
 
+# ── Dependency resolution ─────────────────────────────────────────────────────
+
+class TestServiceComposerDeps(unittest.TestCase):
+
+    def _composer(self):
+        cm = MagicMock()
+        cm.configs = {}
+        cm.get_installed_services.return_value = {}
+        cm.get_identity.return_value = {}
+        cm.get_effective_domain.return_value = 'test.cell'
+        return ServiceComposer(config_manager=cm, data_dir='/tmp/test')
+
+    def test_resolve_requires_no_requires(self):
+        composer = self._composer()
+        manifest = {'id': 'webmail', 'requires': []}
+        result = composer._resolve_requires(manifest, {})
+        self.assertIsNone(result)
+
+    def test_resolve_requires_dep_installed(self):
+        composer = self._composer()
+        manifest = {'id': 'webmail', 'requires': ['email']}
+        installed = {'email': {'manifest': {'id': 'email'}}}
+        result = composer._resolve_requires(manifest, installed)
+        self.assertIsNone(result)
+
+    def test_resolve_requires_dep_missing(self):
+        composer = self._composer()
+        manifest = {'id': 'webmail', 'requires': ['email']}
+        result = composer._resolve_requires(manifest, {})
+        self.assertIsNotNone(result)
+        self.assertIn('email', result)
+
+    def test_resolve_requires_multiple_deps_partial(self):
+        composer = self._composer()
+        manifest = {'id': 'x', 'requires': ['email', 'calendar']}
+        installed = {'email': {'manifest': {'id': 'email'}}}
+        result = composer._resolve_requires(manifest, installed)
+        self.assertIsNotNone(result)
+        self.assertIn('calendar', result)
+        self.assertNotIn('email', result)
+
+    def test_resolve_requires_no_requires_key(self):
+        composer = self._composer()
+        manifest = {'id': 'files'}  # no 'requires' key
+        result = composer._resolve_requires(manifest, {})
+        self.assertIsNone(result)
+
+    def test_resolve_dependents_none(self):
+        composer = self._composer()
+        installed = {
+            'email': {'manifest': {'id': 'email', 'requires': []}},
+        }
+        deps = composer._resolve_dependents('email', installed)
+        self.assertEqual(deps, [])
+
+    def test_resolve_dependents_found(self):
+        composer = self._composer()
+        installed = {
+            'email': {'manifest': {'id': 'email', 'requires': []}},
+            'webmail': {'manifest': {'id': 'webmail', 'requires': ['email']}},
+        }
+        deps = composer._resolve_dependents('email', installed)
+        self.assertIn('webmail', deps)
+
+    def test_resolve_dependents_excludes_self(self):
+        composer = self._composer()
+        installed = {
+            'email': {'manifest': {'id': 'email', 'requires': ['email']}},  # weird edge case
+        }
+        deps = composer._resolve_dependents('email', installed)
+        self.assertNotIn('email', deps)
+
+    def test_resolve_dependents_empty_installed(self):
+        composer = self._composer()
+        deps = composer._resolve_dependents('email', {})
+        self.assertEqual(deps, [])
+
+    def test_reapply_active_services_calls_up(self):
+        cm = MagicMock()
+        cm.get_installed_services.return_value = {'email': {'manifest': {'id': 'email'}}}
+        composer = ServiceComposer(config_manager=cm, data_dir='/tmp/test')
+        composer.has_compose_file = MagicMock(return_value=True)
+        composer.up = MagicMock(return_value={'ok': True})
+        composer.reapply_active_services()
+        composer.up.assert_called_once_with('email')
+
+    def test_reapply_active_services_skips_missing_compose(self):
+        cm = MagicMock()
+        cm.get_installed_services.return_value = {'email': {'manifest': {'id': 'email'}}}
+        composer = ServiceComposer(config_manager=cm, data_dir='/tmp/test')
+        composer.has_compose_file = MagicMock(return_value=False)
+        composer.up = MagicMock()
+        composer.reapply_active_services()
+        composer.up.assert_not_called()
+
+    def test_reapply_active_services_empty(self):
+        cm = MagicMock()
+        cm.get_installed_services.return_value = {}
+        composer = ServiceComposer(config_manager=cm, data_dir='/tmp/test')
+        composer.up = MagicMock()
+        composer.reapply_active_services()
+        composer.up.assert_not_called()
+
+
 if __name__ == '__main__':
     unittest.main()

tests/test_service_store_manager.py

@@ -18,7 +18,6 @@ import yaml
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', 'api'))
 
 from service_store_manager import ServiceStoreManager
-from ip_utils import CONTAINER_OFFSETS
 
 
 # ---------------------------------------------------------------------------
@@ -466,218 +465,6 @@ class TestValidateManifestSubdomain(unittest.TestCase):
         self.assertTrue(ok)
 
 
-# ---------------------------------------------------------------------------
-# _allocate_service_ip
-# ---------------------------------------------------------------------------
-
-class TestAllocateServiceIp(unittest.TestCase):
-
-    def test_first_allocation_skips_reserved_offsets_and_returns_first_free(self):
-        """The first free offset after SERVICE_POOL_START(20) must not be in CONTAINER_OFFSETS."""
-        reserved_offsets = set(CONTAINER_OFFSETS.values())
-        # Find expected first offset (>= 20, not reserved)
-        expected_offset = None
-        for off in range(20, 255):
-            if off not in reserved_offsets:
-                expected_offset = off
-                break
-        expected_ip = f'172.20.0.{expected_offset}'
-
-        mgr = _make_manager()
-        ip = mgr._allocate_service_ip('svc-alpha')
-        self.assertEqual(ip, expected_ip)
-
-    def test_first_allocation_returns_172_20_0_20_for_clean_pool(self):
-        """Offset 20 is not in CONTAINER_OFFSETS, so it should be the first allocated IP."""
-        self.assertNotIn(20, CONTAINER_OFFSETS.values(),
-                         "If offset 20 is now reserved, update this test")
-        mgr = _make_manager()
-        ip = mgr._allocate_service_ip('svc1')
-        self.assertEqual(ip, '172.20.0.20')
-
-    def test_reserved_container_offsets_are_skipped(self):
-        """No allocated IP should land on a CONTAINER_OFFSETS offset."""
-        reserved_offsets = set(CONTAINER_OFFSETS.values())
-        mgr = _make_manager()
-        ip = mgr._allocate_service_ip('svc2')
-        import ipaddress
-        allocated_offset = int(ipaddress.IPv4Address(ip)) - int(ipaddress.IPv4Address('172.20.0.0'))
-        self.assertNotIn(allocated_offset, reserved_offsets)
-
-    def test_already_taken_ips_are_skipped(self):
-        """Already-assigned service IPs in service_ips are not reallocated."""
-        identity = {
-            'ip_range': '172.20.0.0/16',
-            'service_ips': {'svc-existing': '172.20.0.20'},
-        }
-        mgr = _make_manager(identity=identity)
-        ip = mgr._allocate_service_ip('svc-new')
-        # 172.20.0.20 is taken, so must get the next available one
-        self.assertNotEqual(ip, '172.20.0.20')
-        # Should be 172.20.0.21 (offset 21 is vip_calendar in CONTAINER_OFFSETS — skip it)
-        # Find what the next free one should be
-        reserved_offsets = set(CONTAINER_OFFSETS.values())
-        expected_offset = None
-        for off in range(20, 255):
-            if off not in reserved_offsets and f'172.20.0.{off}' != '172.20.0.20':
-                expected_offset = off
-                break
-        self.assertEqual(ip, f'172.20.0.{expected_offset}')
-
-    def test_multiple_taken_ips_skipped_sequentially(self):
-        """Allocator advances past multiple taken IPs correctly."""
-        reserved_offsets = set(CONTAINER_OFFSETS.values())
-        # Pre-fill the first few non-reserved offsets
-        free_offsets = [off for off in range(20, 255) if off not in reserved_offsets]
-        # Take the first 3
-        service_ips = {f'svc{i}': f'172.20.0.{off}' for i, off in enumerate(free_offsets[:3])}
-        identity = {'ip_range': '172.20.0.0/16', 'service_ips': service_ips}
-        mgr = _make_manager(identity=identity)
-        ip = mgr._allocate_service_ip('svc-fourth')
-        self.assertEqual(ip, f'172.20.0.{free_offsets[3]}')
-
-    def test_exhausted_pool_raises_runtime_error(self):
-        """Fill all 20-254 non-reserved offsets and expect RuntimeError."""
-        reserved_offsets = set(CONTAINER_OFFSETS.values())
-        service_ips = {}
-        idx = 0
-        for off in range(20, 255):
-            if off not in reserved_offsets:
-                service_ips[f'svc{idx}'] = f'172.20.0.{off}'
-                idx += 1
-        identity = {'ip_range': '172.20.0.0/16', 'service_ips': service_ips}
-        mgr = _make_manager(identity=identity)
-        with self.assertRaises(RuntimeError) as ctx:
-            mgr._allocate_service_ip('overflow')
-        self.assertIn('exhausted', str(ctx.exception).lower())
-
-    def test_uses_ip_range_from_identity(self):
-        """Allocation respects a different ip_range like 10.10.0.0/16."""
-        identity = {'ip_range': '10.10.0.0/16', 'service_ips': {}}
-        mgr = _make_manager(identity=identity)
-        ip = mgr._allocate_service_ip('svc')
-        self.assertTrue(ip.startswith('10.10.'), f'Expected 10.10.x.x, got {ip}')
-
-
-# ---------------------------------------------------------------------------
-# _render_compose_override
-# ---------------------------------------------------------------------------
-
-class TestRenderComposeOverride(unittest.TestCase):
-
-    def test_empty_records_produces_valid_yaml_with_empty_services(self):
-        mgr = _make_manager()
-        output = mgr._render_compose_override({})
-        doc = yaml.safe_load(output)
-        self.assertIn('services', doc)
-        self.assertEqual(doc['services'], {})
-        self.assertIn('networks', doc)
-        self.assertIn('cell-network', doc['networks'])
-
-    def test_empty_records_has_no_volumes_key(self):
-        mgr = _make_manager()
-        output = mgr._render_compose_override({})
-        doc = yaml.safe_load(output)
-        self.assertNotIn('volumes', doc)
-
-    def test_single_service_renders_correct_definition(self):
-        mgr = _make_manager()
-        records = {
-            'myapp': {
-                'container_name': 'cell-myapp',
-                'service_ip': '172.20.0.20',
-                'manifest': {
-                    'image': 'git.pic.ngo/roof/myapp:1.0',
-                },
-            }
-        }
-        output = mgr._render_compose_override(records)
-        doc = yaml.safe_load(output)
-        svc = doc['services']['cell-myapp']
-        self.assertEqual(svc['image'], 'git.pic.ngo/roof/myapp:1.0')
-        self.assertEqual(svc['container_name'], 'cell-myapp')
-        self.assertEqual(svc['networks']['cell-network']['ipv4_address'], '172.20.0.20')
-        self.assertEqual(svc['restart'], 'unless-stopped')
-
-    def test_named_volumes_declared_at_top_level(self):
-        mgr = _make_manager()
-        records = {
-            'myapp': {
-                'container_name': 'cell-myapp',
-                'service_ip': '172.20.0.20',
-                'manifest': {
-                    'image': 'git.pic.ngo/roof/myapp:1.0',
-                    'volumes': [
-                        {'name': 'myapp-data', 'mount': '/data'},
-                        {'name': 'myapp-config', 'mount': '/config'},
-                    ],
-                },
-            }
-        }
-        output = mgr._render_compose_override(records)
-        doc = yaml.safe_load(output)
-        self.assertIn('volumes', doc)
-        self.assertIn('myapp-data', doc['volumes'])
-        self.assertIn('myapp-config', doc['volumes'])
-
-    def test_named_volumes_appear_in_service_volumes_list(self):
-        mgr = _make_manager()
-        records = {
-            'myapp': {
-                'container_name': 'cell-myapp',
-                'service_ip': '172.20.0.20',
-                'manifest': {
-                    'image': 'git.pic.ngo/roof/myapp:1.0',
-                    'volumes': [{'name': 'myapp-data', 'mount': '/data'}],
-                },
-            }
-        }
-        output = mgr._render_compose_override(records)
-        doc = yaml.safe_load(output)
-        svc_volumes = doc['services']['cell-myapp']['volumes']
-        self.assertIn('myapp-data:/data', svc_volumes)
-
-    def test_environment_rendered_in_service(self):
-        mgr = _make_manager()
-        records = {
-            'myapp': {
-                'container_name': 'cell-myapp',
-                'service_ip': '172.20.0.20',
-                'manifest': {
-                    'image': 'git.pic.ngo/roof/myapp:1.0',
-                    'env': [
-                        {'key': 'FOO', 'value': 'bar'},
-                        {'key': 'PORT', 'value': '8080'},
-                    ],
-                },
-            }
-        }
-        output = mgr._render_compose_override(records)
-        doc = yaml.safe_load(output)
-        env = doc['services']['cell-myapp']['environment']
-        self.assertEqual(env['FOO'], 'bar')
-        self.assertEqual(env['PORT'], '8080')
-
-    def test_no_volumes_key_in_service_when_manifest_has_no_volumes(self):
-        mgr = _make_manager()
-        records = {
-            'myapp': {
-                'container_name': 'cell-myapp',
-                'service_ip': '172.20.0.20',
-                'manifest': {'image': 'git.pic.ngo/roof/myapp:1.0'},
-            }
-        }
-        output = mgr._render_compose_override(records)
-        doc = yaml.safe_load(output)
-        self.assertNotIn('volumes', doc['services']['cell-myapp'])
-
-    def test_network_declared_as_external(self):
-        mgr = _make_manager()
-        output = mgr._render_compose_override({})
-        doc = yaml.safe_load(output)
-        self.assertTrue(doc['networks']['cell-network']['external'])
-
-
 # ---------------------------------------------------------------------------
 # get_status
 # ---------------------------------------------------------------------------
@@ -774,222 +561,200 @@ class TestListServices(unittest.TestCase):
 
 
 # ---------------------------------------------------------------------------
-# install
+# install (new architecture: ServiceComposer-driven)
 # ---------------------------------------------------------------------------
 
+def _make_ssm(config_manager=None, manifest=None, template='version: "3"\nservices: {}\n'):
+    """Build a ServiceStoreManager with a mock service_composer."""
+    cm = config_manager or MagicMock()
+    if config_manager is None:
+        cm.get_installed_services.return_value = {}
+    caddy = MagicMock()
+    composer = MagicMock()
+    composer._resolve_requires.return_value = None  # no missing deps
+    composer._resolve_dependents.return_value = []
+    composer.install.return_value = {'ok': True}
+    ssm = ServiceStoreManager(
+        config_manager=cm,
+        caddy_manager=caddy,
+        container_manager=MagicMock(),
+        service_composer=composer,
+    )
+    if manifest is not None:
+        ssm._fetch_manifest = MagicMock(return_value=manifest)
+        ssm._fetch_template = MagicMock(return_value=template)
+    return ssm, cm, caddy, composer
+
+
 class TestInstall(unittest.TestCase):
 
-    def _mock_fetch(self, mgr, manifest):
-        mgr._fetch_manifest = MagicMock(return_value=manifest)
-
-    def _mock_write_compose(self, mgr):
-        mgr._write_compose_override = MagicMock()
-
     def test_install_already_installed_returns_ok_already_installed(self):
-        installed = {'myapp': {'id': 'myapp'}}
+        cm = MagicMock()
-        mgr = _make_manager(installed=installed)
+        cm.get_installed_services.return_value = {'myapp': {'id': 'myapp'}}
-        with patch('firewall_manager.apply_service_rules'):
+        ssm, _, _, _ = _make_ssm(config_manager=cm)
-            result = mgr.install('myapp')
+        result = ssm.install('myapp')
         self.assertTrue(result['ok'])
         self.assertTrue(result.get('already_installed'))
 
-    def test_install_invalid_manifest_returns_errors(self):
-        mgr = _make_manager()
-        bad_manifest = {'id': 'myapp', 'image': 'bad-registry.io/img:latest'}
-        self._mock_fetch(mgr, bad_manifest)
-        self._mock_write_compose(mgr)
-        with patch('firewall_manager.apply_service_rules'):
-            result = mgr.install('myapp')
-        self.assertFalse(result['ok'])
-        self.assertIn('errors', result)
-
-    def test_install_valid_manifest_returns_ok_true(self):
-        mgr = _make_manager()
-        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
-        self._mock_fetch(mgr, manifest)
-        self._mock_write_compose(mgr)
-        with patch('firewall_manager.apply_service_rules'), \
-             patch('service_store_manager.subprocess.run') as mock_run:
-            mock_run.return_value = MagicMock(returncode=0, stderr='')
-            result = mgr.install('myapp')
-        self.assertTrue(result['ok'])
-        self.assertFalse(result.get('already_installed', False))
-
-    def test_install_returns_service_ip(self):
-        mgr = _make_manager()
-        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
-        self._mock_fetch(mgr, manifest)
-        self._mock_write_compose(mgr)
-        with patch('firewall_manager.apply_service_rules'), \
-             patch('service_store_manager.subprocess.run') as mock_run:
-            mock_run.return_value = MagicMock(returncode=0, stderr='')
-            result = mgr.install('myapp')
-        self.assertIn('service_ip', result)
-        self.assertTrue(result['service_ip'].startswith('172.20.'))
-
-    def test_install_returns_container_name(self):
-        mgr = _make_manager()
-        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
-        self._mock_fetch(mgr, manifest)
-        self._mock_write_compose(mgr)
-        with patch('firewall_manager.apply_service_rules'), \
-             patch('service_store_manager.subprocess.run') as mock_run:
-            mock_run.return_value = MagicMock(returncode=0, stderr='')
-            result = mgr.install('myapp')
-        self.assertEqual(result['container_name'], 'cell-myapp')
-
-    def test_install_calls_set_installed_service(self):
-        mgr = _make_manager()
-        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
-        self._mock_fetch(mgr, manifest)
-        self._mock_write_compose(mgr)
-        with patch('firewall_manager.apply_service_rules'), \
-             patch('service_store_manager.subprocess.run') as mock_run:
-            mock_run.return_value = MagicMock(returncode=0, stderr='')
-            mgr.install('myapp')
-        mgr.config_manager.set_installed_service.assert_called_once()
-        args = mgr.config_manager.set_installed_service.call_args[0]
-        self.assertEqual(args[0], 'myapp')
-
-    def test_install_calls_caddy_regenerate_when_service_has_caddy_route(self):
-        mgr = _make_manager()
-        manifest = _valid_manifest(
-            id='myapp',
-            container_name='cell-myapp',
-            caddy_route={'subdomain': 'myapp', 'upstream': 'cell-myapp:8080'},
-        )
-        self._mock_fetch(mgr, manifest)
-        self._mock_write_compose(mgr)
-        with patch('firewall_manager.apply_service_rules'), \
-             patch('service_store_manager.subprocess.run') as mock_run:
-            mock_run.return_value = MagicMock(returncode=0, stderr='')
-            mgr.install('myapp')
-        mgr.caddy_manager.regenerate_with_installed.assert_called()
-
-    def test_install_saves_service_ip_in_identity(self):
-        mgr = _make_manager()
-        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
-        self._mock_fetch(mgr, manifest)
-        self._mock_write_compose(mgr)
-        with patch('firewall_manager.apply_service_rules'), \
-             patch('service_store_manager.subprocess.run') as mock_run:
-            mock_run.return_value = MagicMock(returncode=0, stderr='')
-            mgr.install('myapp')
-        mgr.config_manager.set_identity_field.assert_called()
-        call_args = mgr.config_manager.set_identity_field.call_args[0]
-        self.assertEqual(call_args[0], 'service_ips')
-        self.assertIn('myapp', call_args[1])
-
     def test_install_fetch_failure_returns_error(self):
-        mgr = _make_manager()
+        ssm, _, _, _ = _make_ssm()
-        mgr._fetch_manifest = MagicMock(side_effect=Exception('connection refused'))
+        ssm._fetch_manifest = MagicMock(side_effect=Exception('connection refused'))
-        result = mgr.install('nonexistent')
+        result = ssm.install('myapp')
         self.assertFalse(result['ok'])
         self.assertIn('error', result)
         self.assertIn('fetch', result['error'].lower())
 
+    def test_install_invalid_manifest_returns_errors(self):
+        bad_manifest = {'id': 'myapp', 'image': 'bad-registry.io/img:latest'}
+        ssm, _, _, _ = _make_ssm(manifest=bad_manifest)
+        result = ssm.install('myapp')
+        self.assertFalse(result['ok'])
+        self.assertIn('errors', result)
+
+    def test_install_missing_dep_returns_error(self):
+        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
+        ssm, _, _, composer = _make_ssm(manifest=manifest)
+        composer._resolve_requires.return_value = 'Required services not installed: email'
+        result = ssm.install('myapp')
+        self.assertFalse(result['ok'])
+        self.assertIn('error', result)
+        self.assertIn('email', result['error'])
+
+    def test_install_template_fetch_failure_returns_error(self):
+        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
+        ssm, _, _, _ = _make_ssm(manifest=manifest)
+        ssm._fetch_template = MagicMock(side_effect=Exception('404 Not Found'))
+        result = ssm.install('myapp')
+        self.assertFalse(result['ok'])
+        self.assertIn('error', result)
+        self.assertIn('compose template', result['error'].lower())
+
+    def test_install_composer_install_failure_returns_error(self):
+        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
+        ssm, _, _, composer = _make_ssm(manifest=manifest)
+        composer.install.return_value = {'ok': False, 'stderr': 'docker error'}
+        result = ssm.install('myapp')
+        self.assertFalse(result['ok'])
+        self.assertIn('error', result)
+
+    def test_install_calls_set_installed_service(self):
+        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
+        ssm, cm, _, _ = _make_ssm(manifest=manifest)
+        ssm.install('myapp')
+        cm.set_installed_service.assert_called_once()
+        args = cm.set_installed_service.call_args[0]
+        self.assertEqual(args[0], 'myapp')
+
+    def test_install_record_contains_manifest(self):
+        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
+        ssm, cm, _, _ = _make_ssm(manifest=manifest)
+        ssm.install('myapp')
+        record = cm.set_installed_service.call_args[0][1]
+        self.assertIn('manifest', record)
+
+    def test_install_calls_caddy_regenerate(self):
+        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
+        ssm, _, caddy, _ = _make_ssm(manifest=manifest)
+        ssm.install('myapp')
+        caddy.regenerate_with_installed.assert_called()
+
+    def test_install_returns_ok_true(self):
+        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
+        ssm, _, _, _ = _make_ssm(manifest=manifest)
+        result = ssm.install('myapp')
+        self.assertTrue(result['ok'])
+        self.assertFalse(result.get('already_installed', False))
+
+    def test_install_without_composer_stores_record(self):
+        """When service_composer=None, skip compose but still store the install record."""
+        manifest = _valid_manifest(id='myapp', container_name='cell-myapp')
+        cm = MagicMock()
+        cm.get_installed_services.return_value = {}
+        caddy = MagicMock()
+        ssm = ServiceStoreManager(
+            config_manager=cm,
+            caddy_manager=caddy,
+            container_manager=MagicMock(),
+            service_composer=None,
+        )
+        ssm._fetch_manifest = MagicMock(return_value=manifest)
+        ssm._fetch_template = MagicMock(return_value='version: "3"\nservices: {}\n')
+        result = ssm.install('myapp')
+        self.assertTrue(result['ok'])
+        cm.set_installed_service.assert_called_once()
+
 
 # ---------------------------------------------------------------------------
-# remove
+# remove (new architecture: ServiceComposer-driven)
 # ---------------------------------------------------------------------------
 
 class TestRemove(unittest.TestCase):
 
-    def _mgr_with_installed(self, tmp_dir, service_id='myapp'):
+    def _make_mgr_with_installed(self, service_id='myapp'):
         record = {
-            'container_name': 'cell-myapp',
+            'id': service_id,
-            'service_ip': '172.20.0.20',
+            'manifest': {'id': service_id, 'image': 'git.pic.ngo/roof/myapp:1.0'},
-            'manifest': {'image': 'git.pic.ngo/roof/myapp:1.0', 'volumes': []},
-            'iptables_rules': [],
         }
         installed = {service_id: record}
-        mgr = _make_manager(tmp_dir=tmp_dir, installed=installed)
+        cm = MagicMock()
-        # After remove, config_manager.get_installed_services returns empty
+        cm.get_installed_services.return_value = installed
-        mgr.config_manager.remove_installed_service = MagicMock()
+        caddy = MagicMock()
-        mgr.config_manager.get_installed_services.side_effect = [
+        composer = MagicMock()
-            installed,    # first call (inside remove, initial check)
+        composer._resolve_dependents.return_value = []
-            {},           # second call (after removal, for compose rewrite)
+        composer.remove.return_value = {'ok': True}
-        ]
+        ssm = ServiceStoreManager(
-        mgr._write_compose_override = MagicMock()
+            config_manager=cm,
-        return mgr
+            caddy_manager=caddy,
+            container_manager=MagicMock(),
+            service_composer=composer,
+        )
+        return ssm, cm, caddy, composer
 
     def test_remove_not_installed_returns_error(self):
-        mgr = _make_manager()
+        cm = MagicMock()
-        with patch('firewall_manager.clear_service_rules'):
+        cm.get_installed_services.return_value = {}
-            result = mgr.remove('nosuchapp')
+        ssm = ServiceStoreManager(
+            config_manager=cm,
+            caddy_manager=MagicMock(),
+            container_manager=MagicMock(),
+        )
+        result = ssm.remove('nosuchapp')
         self.assertFalse(result['ok'])
         self.assertIn('error', result)
         self.assertIn('not installed', result['error'])
 
-    def test_remove_installed_returns_ok_true(self, tmp_dir='/tmp/pic-ssm-rm-test'):
+    def test_remove_with_dependents_returns_error(self):
-        import tempfile, shutil
+        ssm, _, _, composer = self._make_mgr_with_installed()
-        tmp = tempfile.mkdtemp()
+        composer._resolve_dependents.return_value = ['webmail']
-        try:
+        result = ssm.remove('myapp')
-            mgr = self._mgr_with_installed(tmp)
+        self.assertFalse(result['ok'])
-            with patch('firewall_manager.clear_service_rules'), \
+        self.assertIn('error', result)
-                 patch('service_store_manager.subprocess.run') as mock_run:
+        self.assertIn('webmail', result['error'])
-                mock_run.return_value = MagicMock(returncode=0, stderr='')
+
-                result = mgr.remove('myapp')
+    def test_remove_calls_composer_remove(self):
-            self.assertTrue(result['ok'])
+        ssm, _, _, composer = self._make_mgr_with_installed()
-        finally:
+        ssm.remove('myapp')
-            shutil.rmtree(tmp, ignore_errors=True)
+        composer.remove.assert_called_once_with('myapp', purge_data=False)
 
     def test_remove_calls_remove_installed_service(self):
-        import tempfile, shutil
+        ssm, cm, _, _ = self._make_mgr_with_installed()
-        tmp = tempfile.mkdtemp()
+        ssm.remove('myapp')
-        try:
+        cm.remove_installed_service.assert_called_once_with('myapp')
-            mgr = self._mgr_with_installed(tmp)
-            with patch('firewall_manager.clear_service_rules'), \
-                 patch('service_store_manager.subprocess.run') as mock_run:
-                mock_run.return_value = MagicMock(returncode=0, stderr='')
-                mgr.remove('myapp')
-            mgr.config_manager.remove_installed_service.assert_called_once_with('myapp')
-        finally:
-            shutil.rmtree(tmp, ignore_errors=True)
 
     def test_remove_calls_caddy_regenerate(self):
-        import tempfile, shutil
+        ssm, _, caddy, _ = self._make_mgr_with_installed()
-        tmp = tempfile.mkdtemp()
+        ssm.remove('myapp')
-        try:
+        caddy.regenerate_with_installed.assert_called()
-            mgr = self._mgr_with_installed(tmp)
-            with patch('firewall_manager.clear_service_rules'), \
-                 patch('service_store_manager.subprocess.run') as mock_run:
-                mock_run.return_value = MagicMock(returncode=0, stderr='')
-                mgr.remove('myapp')
-            mgr.caddy_manager.regenerate_with_installed.assert_called()
-        finally:
-            shutil.rmtree(tmp, ignore_errors=True)
 
-    def test_remove_purge_data_calls_docker_volume_rm(self):
+    def test_remove_returns_ok_true(self):
-        import tempfile, shutil
+        ssm, _, _, _ = self._make_mgr_with_installed()
-        tmp = tempfile.mkdtemp()
+        result = ssm.remove('myapp')
-        try:
+        self.assertTrue(result['ok'])
-            record = {
+
-                'container_name': 'cell-myapp',
+    def test_remove_purge_data_passed_to_composer(self):
-                'service_ip': '172.20.0.20',
+        ssm, _, _, composer = self._make_mgr_with_installed()
-                'manifest': {
+        ssm.remove('myapp', purge_data=True)
-                    'image': 'git.pic.ngo/roof/myapp:1.0',
+        composer.remove.assert_called_once_with('myapp', purge_data=True)
-                    'volumes': [{'name': 'myapp-data', 'mount': '/data'}],
-                },
-            }
-            mgr = _make_manager(tmp_dir=tmp, installed={'myapp': record})
-            mgr.config_manager.remove_installed_service = MagicMock()
-            mgr.config_manager.get_installed_services.side_effect = [
-                {'myapp': record}, {}
-            ]
-            mgr._write_compose_override = MagicMock()
-            with patch('firewall_manager.clear_service_rules'), \
-                 patch('service_store_manager.subprocess.run') as mock_run:
-                mock_run.return_value = MagicMock(returncode=0, stderr='')
-                mgr.remove('myapp', purge_data=True)
-            # Check that docker volume rm was called with the volume name
-            calls = [str(c) for c in mock_run.call_args_list]
-            self.assertTrue(
-                any('myapp-data' in c for c in calls),
-                f'Expected docker volume rm myapp-data in calls: {calls}',
-            )
-        finally:
-            shutil.rmtree(tmp, ignore_errors=True)
 
 
 if __name__ == '__main__':