diff --git a/docs/site-configuration.md b/docs/site-configuration.md index 8d14c4c..2443d05 100644 --- a/docs/site-configuration.md +++ b/docs/site-configuration.md @@ -174,7 +174,49 @@ subscription: "real-subscription-id" resourceGroup: real-resource-group ``` -> **Security**: Only base files (`sites/`) can specify `inherits`. Overlays cannot inject inheritance. +> **Security**: Only base files (in trusted site directories) can specify `inherits`. Overlays in `sites.local/` cannot inject inheritance. + +## Extra trusted site directories + +In addition to the workspace's `sites/` directory, Site Ops can search +one or more extra trusted directories for site files. Files in these +directories are treated exactly like files in `sites/`: they are +discoverable by `siteops sites`, they can declare `inherits`, and they +serve as valid base files for the inheritance chain. + +Use cases include: + +- **CI / end-to-end tests**: keep test-only sites out of `workspaces/*/sites/` + (production config) and inject them only when the test workflow runs. +- **Cross-repo site libraries**: pull shared sites from another repository + checked out alongside the workspace. +- **Blueprint catalogs**: keep opinionated site templates in a central + location, pointed at from multiple workspaces. + +Provide extra directories via the CLI or environment variable: + +```bash +# Repeatable flag +siteops -w workspace --extra-sites-dir ./tests/e2e/sites sites + +# Environment variable (os.pathsep-separated: ';' on Windows, ':' on Unix) +SITEOPS_EXTRA_SITES_DIRS=/path/to/lib-sites siteops -w workspace sites +``` + +When both are provided, the CLI flag wins and an INFO log records that +the env var was ignored. + +**Merge order (full)**: + +``` +inherits target → sites/ → → sites.local/ +``` + +Extras cannot collide with the workspace's own `sites/` or `sites.local/` +directories; the orchestrator rejects both at construction time. +Registering `sites.local/` as trusted is specifically refused because it +would let overlays inject inheritance and break the overlay security +invariant. ## Site inheritance @@ -227,8 +269,8 @@ parameters: ### Merge order with inheritance -`inherits target` → `sites/` → `sites.local/` +`inherits target` → `sites/` → `` → `sites.local/` -Inherited values are overridden by child site values. Nested objects (labels, parameters, properties) merge recursively. +Inherited values are overridden by child site values. Nested objects (labels, parameters, properties) merge recursively. See [Extra trusted site directories](#extra-trusted-site-directories) for how extra dirs participate in the chain. -> **Security**: Only base files (`sites/`) can specify `inherits`. Overlays cannot inject inheritance. +> **Security**: Only base files (in trusted site directories) can specify `inherits`. Overlays in `sites.local/` cannot inject inheritance, even when extra trusted dirs are configured. diff --git a/siteops/cli.py b/siteops/cli.py index 6bb524e..dacc319 100644 --- a/siteops/cli.py +++ b/siteops/cli.py @@ -11,6 +11,7 @@ import argparse import logging +import os import sys from pathlib import Path from typing import Any @@ -204,6 +205,40 @@ def cmd_sites(args: argparse.Namespace, orchestrator: Orchestrator) -> int: return 0 +_EXTRA_SITES_DIRS_ENV = "SITEOPS_EXTRA_SITES_DIRS" + + +def _resolve_extra_sites_dirs(cli_dirs: list[Path] | None) -> list[Path]: + """Resolve extra trusted site dirs from CLI flag and/or env var. + + Precedence: `--extra-sites-dir` wins over `SITEOPS_EXTRA_SITES_DIRS`. + When both are provided, an INFO log records that the env var was ignored. + + The env var is parsed using `os.pathsep` (`;` on Windows, `:` on + Unix) to match platform conventions for `PATH`-style variables. Empty + segments are skipped so trailing separators are tolerated. + + Args: + cli_dirs: Directories supplied via the `--extra-sites-dir` flag, + or `None` if the flag was not used. + + Returns: + List of paths to pass to `Orchestrator`. Empty list when neither + source provides a value. + """ + env_raw = os.environ.get(_EXTRA_SITES_DIRS_ENV, "") + env_dirs = [Path(p) for p in env_raw.split(os.pathsep) if p] + + if cli_dirs: + if env_dirs: + logging.getLogger("siteops.cli").info( + "Ignoring %s (--extra-sites-dir takes precedence).", + _EXTRA_SITES_DIRS_ENV, + ) + return list(cli_dirs) + return env_dirs + + def main() -> None: """Main entry point for the Site Ops CLI.""" parser = argparse.ArgumentParser( @@ -227,6 +262,24 @@ def main() -> None: default=Path.cwd(), help="Workspace directory (default: current directory)", ) + parser.add_argument( + "--extra-sites-dir", + dest="extra_sites_dirs", + action="append", + type=Path, + default=None, + metavar="DIR", + help=( + "Additional trusted directory to search for site YAML files " + "(repeatable). Treated with the same trust level as the " + "workspace's sites/ directory: files may declare 'inherits'. " + "Searched after sites/ and before sites.local/. " + "Alternatively set SITEOPS_EXTRA_SITES_DIRS to a list of " + "directories using the platform path separator " + "(';' on Windows, ':' on Unix). When both are provided, " + "--extra-sites-dir wins." + ), + ) subparsers = parser.add_subparsers(dest="command", required=True) @@ -308,10 +361,17 @@ def main() -> None: print(f"Error: Workspace directory not found: {args.workspace}", file=sys.stderr) sys.exit(1) - orchestrator = Orchestrator( - workspace=args.workspace, - dry_run=getattr(args, "dry_run", False), - ) + extra_sites_dirs = _resolve_extra_sites_dirs(args.extra_sites_dirs) + + try: + orchestrator = Orchestrator( + workspace=args.workspace, + dry_run=getattr(args, "dry_run", False), + extra_trusted_sites_dirs=extra_sites_dirs, + ) + except (FileNotFoundError, ValueError) as e: + print(f"Error: {e}", file=sys.stderr) + sys.exit(1) commands = { "deploy": cmd_deploy, diff --git a/siteops/orchestrator.py b/siteops/orchestrator.py index 16f4386..bfdff5d 100644 --- a/siteops/orchestrator.py +++ b/siteops/orchestrator.py @@ -121,7 +121,12 @@ class Orchestrator: executor: The AzCliExecutor instance for running commands """ - def __init__(self, workspace: Path, dry_run: bool = False): + def __init__( + self, + workspace: Path, + dry_run: bool = False, + extra_trusted_sites_dirs: list[Path] | None = None, + ): self.workspace = Path(workspace).resolve() self.dry_run = dry_run self.executor = AzCliExecutor(workspace=self.workspace, dry_run=dry_run) @@ -129,6 +134,81 @@ def __init__(self, workspace: Path, dry_run: bool = False): self._params_cache_lock = threading.Lock() self._site_cache: dict[str, Site] = {} self._cache_lock = threading.Lock() + self._extra_trusted_sites_dirs = self._normalize_extra_sites_dirs( + extra_trusted_sites_dirs or [] + ) + + def _normalize_extra_sites_dirs(self, dirs: list[Path]) -> list[Path]: + """Validate and deduplicate extra trusted site directories. + + Extra trusted dirs are searched between the workspace's `sites/` and + `sites.local/` directories, and receive the same trust level as + `sites/`: site files in them are allowed to declare `inherits`. + + Args: + dirs: Candidate directories to add to the trusted search path. + + Returns: + Resolved, deduplicated, order-preserving list. + + Raises: + FileNotFoundError: If any directory does not exist. + ValueError: If a directory collides with the workspace's own + `sites/` or `sites.local/`. A `sites.local/` collision + is specifically refused because registering it as trusted + would let overlays inject inheritance, breaking the overlay + security invariant. + """ + primary = (self.workspace / "sites").resolve() + overlay = (self.workspace / "sites.local").resolve() + result: list[Path] = [] + seen: set[Path] = set() + for candidate in dirs: + resolved = Path(candidate).resolve() + if not resolved.is_dir(): + raise FileNotFoundError( + f"Extra trusted site directory not found: {candidate}" + ) + if resolved == primary: + raise ValueError( + f"Extra site dir '{candidate}' is the workspace's " + f"sites/ directory; already included by default." + ) + if resolved == overlay: + raise ValueError( + f"Extra site dir '{candidate}' is the workspace's " + f"sites.local/ directory. Registering it as trusted " + f"would allow overlays to inject inheritance; refused " + f"for security." + ) + if resolved in seen: + continue + seen.add(resolved) + result.append(resolved) + return result + + @property + def _trusted_sites_dirs(self) -> list[Path]: + """All trusted site directories, in merge order. + + Trusted means: `inherits` is honored in files from these dirs. + Excludes `sites.local/` (overlay, always strips `inherits`). + """ + return [self.workspace / "sites", *self._extra_trusted_sites_dirs] + + def _find_trusted_site_file(self, name: str) -> Path | None: + """Return the first trusted directory path containing `.yaml`. + + Searches `sites/` and each extra trusted directory in order. + Does NOT search `sites.local/`: sites must be declared in a + code-reviewed (or caller-vouched-for) location to be loadable. + """ + for sites_dir in self._trusted_sites_dirs: + for ext in (".yaml", ".yml"): + path = sites_dir / f"{name}{ext}" + if path.exists(): + return path + return None def _deep_merge(self, base: dict[str, Any], override: dict[str, Any]) -> dict[str, Any]: """Deep merge two dictionaries, with override taking precedence. @@ -214,31 +294,40 @@ def _load_site_data(self, name: str) -> dict[str, Any]: """Load and merge site data with inheritance and overlay support. Merge order (later overrides earlier): - 1. inherits target - Parent template (if specified, resolved recursively) - 2. sites/ - Base site definitions (committed) - 3. sites.local/ - Local/CI overrides (gitignored) - - Note: Only the base file (sites/) can specify `inherits`. The sites.local/ - overlay cannot change inheritance for security reasons. + 1. inherits target - Parent template (resolved recursively) + 2. sites/ - Primary trusted site definitions (committed) + 3. extra_trusted_sites_dirs - Additional trusted dirs, in list order + 4. sites.local/ - Local/CI overrides (gitignored) + + `inherits` handling: + - The FIRST trusted directory to contain the site establishes the + inheritance chain (`inherits` is honored). + - Any later file (in another trusted dir OR in `sites.local/`) has + its `inherits` stripped. A site has exactly one inheritance + chain, determined by its base file. + + This means `sites.local/` cannot inject inheritance at all: the + security invariant is preserved regardless of how many extra trusted + dirs are configured. Args: - name: Site name (filename without extension) + name: Site name (filename without extension). Returns: - Merged site data dictionary + Merged site data dictionary. Raises: - FileNotFoundError: If site file doesn't exist in any directory - ValueError: If inheritance creates a cycle or references invalid kind + FileNotFoundError: If no trusted dir or sites.local/ has the file. + ValueError: If inheritance creates a cycle or references invalid kind. """ site_dirs = [ - self.workspace / "sites", # Base (committed) - self.workspace / "sites.local", # Local/CI overrides + *self._trusted_sites_dirs, + self.workspace / "sites.local", ] merged_data: dict[str, Any] = {} found = False - is_base_file = True # Track if we're processing the base file + is_base_file = True # First file found establishes the inheritance chain for sites_dir in site_dirs: for ext in (".yaml", ".yml"): @@ -247,7 +336,7 @@ def _load_site_data(self, name: str) -> dict[str, Any]: with open(path, "r", encoding="utf-8") as f: data = yaml.safe_load(f) or {} - # Process inheritance only on base file (not local overlay) + # Process inheritance only on the first file found (the base) if is_base_file and "inherits" in data: inherits_path = (path.parent / data["inherits"]).resolve() # Initialize seen list with current file to detect self-reference @@ -256,7 +345,11 @@ def _load_site_data(self, name: str) -> dict[str, Any]: # Remove inherits from data before merging data = {k: v for k, v in data.items() if k != "inherits"} elif not is_base_file and "inherits" in data: - # Strip inherits from local overlay (security: can't inject inheritance) + # Strip inherits from any non-base file. For sites.local/ + # this prevents runtime injection of inheritance (security). + # For additional trusted dirs it reflects the rule that a + # site has exactly one inheritance chain, established by + # the base file. data = {k: v for k, v in data.items() if k != "inherits"} merged_data = self._deep_merge(merged_data, data) @@ -266,7 +359,11 @@ def _load_site_data(self, name: str) -> dict[str, Any]: break # Only load one file per directory (prefer .yaml) if not found: - raise FileNotFoundError(f"Site '{name}' not found in sites/ or sites.local/") + where = "sites/" + if self._extra_trusted_sites_dirs: + where += ", extra trusted sites dirs," + where += " or sites.local/" + raise FileNotFoundError(f"Site '{name}' not found in {where}") return merged_data @@ -274,33 +371,39 @@ def load_site(self, name: str) -> Site: """Load a site by name, applying inheritance and local overlays. Resolution order (later sources override earlier): - 1. Inherited site/template (if 'inherits' specified) - 2. Base site file from sites/{name}.yaml - 3. Local overlay from sites.local/{name}.yaml (if exists) + 1. Inherited site/template (if 'inherits' specified on the base file). + 2. Base site file from `sites/` or any extra trusted dir (first + trusted dir containing the file wins; see `_find_trusted_site_file`). + 3. Overlays from any remaining trusted dirs (`inherits` stripped). + 4. Local overlay from `sites.local/{name}.yaml` (if it exists; + `inherits` stripped). Args: - name: Site name (corresponds to sites/{name}.yaml) + name: Site name (filename stem, without extension). Returns: - Fully resolved Site instance + Fully resolved Site instance. Raises: ValueError: If site file is invalid, missing required fields, - or references non-existent inherited files - FileNotFoundError: If sites/{name}.yaml doesn't exist + or references non-existent inherited files. + FileNotFoundError: If `{name}.yaml` is not found in `sites/` + or any extra trusted directory. """ # Check cache first with self._cache_lock: if name in self._site_cache: return self._site_cache[name] - # Check if site file exists - site_path = self.workspace / "sites" / f"{name}.yaml" - if not site_path.exists(): - # Also check .yml extension - site_path = self.workspace / "sites" / f"{name}.yml" - if not site_path.exists(): - raise FileNotFoundError(f"Site file not found: sites/{name}.yaml") + # Locate the site file in a trusted directory (sites/ or extras). + # sites.local/ cannot be the primary source. Sites must be declared + # in a code-reviewed / caller-vouched-for location. + site_path = self._find_trusted_site_file(name) + if site_path is None: + where = "sites/" + if self._extra_trusted_sites_dirs: + where += " or extra trusted sites dirs" + raise FileNotFoundError(f"Site file not found: {name} (searched {where})") # Check if this is a SiteTemplate (cannot be loaded directly) if self._is_site_template(site_path): @@ -348,30 +451,33 @@ def load_site(self, name: str) -> Site: return site def _get_all_site_names(self) -> list[str]: - """Get all deployable site names from the sites directory. + """Get all deployable site names from trusted site directories. - Scans the workspace's sites/ directory for YAML files and returns - the names of files that represent deployable sites (kind: Site). - Files with kind: SiteTemplate are excluded as they are only used - for inheritance. + Scans the workspace's `sites/` directory and every extra trusted + site directory for YAML files and returns names of files that + represent deployable sites (`kind: Site`). Files with + `kind: SiteTemplate` are excluded (inheritance-only). Files in + `sites.local/` are NOT discoverable. That directory is the + overlay for committed/trusted sites, not a source of new site + identities. Returns: - List of site names (filename stems without .yaml extension) + Sorted list of site names (filename stems without extension). Note: - - Files that cannot be parsed are included and will error during load_site() - - This allows proper error reporting with full context + Files that cannot be parsed are included and will error during + `load_site()`. This allows proper error reporting with full + context rather than silent omission. """ - sites_dir = self.workspace / "sites" - if not sites_dir.exists(): - return [] - - site_names = set() # Use set to avoid duplicates if both .yaml and .yml exist - for ext in ("*.yaml", "*.yml"): - for path in sites_dir.glob(ext): - if self._is_site_template(path): - continue - site_names.add(path.stem) + site_names: set[str] = set() + for sites_dir in self._trusted_sites_dirs: + if not sites_dir.exists(): + continue + for ext in ("*.yaml", "*.yml"): + for path in sites_dir.glob(ext): + if self._is_site_template(path): + continue + site_names.add(path.stem) return sorted(site_names) # Sort for deterministic order @@ -397,13 +503,15 @@ def _is_site_template(self, path: Path) -> bool: return False def load_all_sites(self) -> list[Site]: - """Load all sites from sites/ and sites.local/ directories. + """Load all deployable sites from trusted site directories. - Sites are merged across directories with the following precedence: - sites.local/ > sites/ + Discovers sites from `sites/` and any extra trusted directories, + then loads each (applying `sites.local/` overlays where present). + Precedence within a single site: `sites.local/` > extra trusted + dirs (last wins) > `sites/`. Returns: - List of all Site instances found (with merged configuration) + List of all Site instances found (with merged configuration). """ sites = [] skipped = [] diff --git a/tests/test_orchestrator_extra_sites_dirs.py b/tests/test_orchestrator_extra_sites_dirs.py new file mode 100644 index 0000000..1c0fb68 --- /dev/null +++ b/tests/test_orchestrator_extra_sites_dirs.py @@ -0,0 +1,302 @@ +"""Tests for the `extra_trusted_sites_dirs` orchestrator feature. + +Covers: +- Behavioral: site discovery, inheritance, precedence, ordering. +- Validation: non-existent dirs, collisions with sites/ and sites.local/, + de-duplication. +- CLI plumbing: `--extra-sites-dir` flag, `SITEOPS_EXTRA_SITES_DIRS` + env var, precedence between them. +""" + +from __future__ import annotations + +import os +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml + +from siteops.cli import _resolve_extra_sites_dirs +from siteops.orchestrator import Orchestrator + + +def _write_site(path: Path, **overrides) -> None: + """Write a minimal Site YAML file, applying overrides. + + Pass `key=None` in overrides to omit that default field entirely + (useful when exercising inheritance). + """ + data = { + "apiVersion": "siteops/v1", + "kind": "Site", + "name": path.stem, + "subscription": "00000000-0000-0000-0000-000000000000", + "resourceGroup": f"rg-{path.stem}", + "location": "eastus", + } + data.update(overrides) + data = {k: v for k, v in data.items() if v is not None} + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(yaml.dump(data)) + + +def _write_template(path: Path, **overrides) -> None: + """Write a minimal SiteTemplate YAML file, applying overrides.""" + data = { + "apiVersion": "siteops/v1", + "kind": "SiteTemplate", + } + data.update(overrides) + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(yaml.dump(data)) + + +class TestExtraSitesDirsBehavior: + """Behavioral tests for extra trusted site directories.""" + + def test_site_in_extra_dir_is_discoverable(self, tmp_workspace, tmp_path): + extra = tmp_path / "extra-sites" + extra.mkdir() + _write_site(extra / "remote-site.yaml") + + orchestrator = Orchestrator(tmp_workspace, extra_trusted_sites_dirs=[extra]) + + site = orchestrator.load_site("remote-site") + assert site.name == "remote-site" + assert "remote-site" in {s.name for s in orchestrator.load_all_sites()} + + def test_site_in_extra_dir_honors_inherits(self, tmp_workspace, tmp_path): + """Files in extra dirs are trusted: inherits is preserved.""" + extra = tmp_path / "extra-sites" + extra.mkdir() + shared = tmp_path / "shared" + shared.mkdir() + + _write_template( + shared / "base.yaml", + subscription="inherited-sub", + labels={"team": "platform"}, + ) + _write_site( + extra / "child.yaml", + subscription=None, + inherits="../shared/base.yaml", + labels={"environment": "prod"}, + ) + + orchestrator = Orchestrator(tmp_workspace, extra_trusted_sites_dirs=[extra]) + site = orchestrator.load_site("child") + + assert site.subscription == "inherited-sub" + assert site.labels["team"] == "platform" + assert site.labels["environment"] == "prod" + + def test_inherits_target_resolved_relative_to_site_file( + self, tmp_workspace, tmp_path + ): + """A site in sites/ can inherit a template located next to an extra dir.""" + extra = tmp_path / "extra-sites" + extra.mkdir() + _write_template(extra / "base.yaml", subscription="from-extra") + + # site in workspace sites/ inherits from the extra dir + _write_site( + tmp_workspace / "sites" / "local-child.yaml", + subscription=None, + inherits=str((extra / "base.yaml").resolve()), + ) + + orchestrator = Orchestrator(tmp_workspace, extra_trusted_sites_dirs=[extra]) + site = orchestrator.load_site("local-child") + assert site.subscription == "from-extra" + + def test_sites_dir_wins_over_extra_on_same_name(self, tmp_workspace, tmp_path): + """When a site name exists in both sites/ and an extra dir, sites/ wins + as the base file (establishes the inheritance chain), but the extra + dir file is still merged as an overlay with inherits stripped. + """ + extra = tmp_path / "extra-sites" + extra.mkdir() + _write_site( + tmp_workspace / "sites" / "dup.yaml", + location="eastus", + labels={"source": "primary"}, + ) + _write_site( + extra / "dup.yaml", + location="westus", + labels={"source": "extra", "extra-only": "yes"}, + ) + + orchestrator = Orchestrator(tmp_workspace, extra_trusted_sites_dirs=[extra]) + site = orchestrator.load_site("dup") + + # Extra dir overlays after sites/: its values win for overlapping keys. + assert site.location == "westus" + assert site.labels["source"] == "extra" + # Keys only in extra are added. + assert site.labels["extra-only"] == "yes" + + def test_multiple_extra_dirs_later_overrides_earlier( + self, tmp_workspace, tmp_path + ): + first = tmp_path / "first" + second = tmp_path / "second" + first.mkdir() + second.mkdir() + + _write_site(first / "shared.yaml", location="eastus") + _write_site(second / "shared.yaml", location="westeurope") + + orchestrator = Orchestrator( + tmp_workspace, extra_trusted_sites_dirs=[first, second] + ) + site = orchestrator.load_site("shared") + + # 'second' is listed after 'first' so it overlays on top. + assert site.location == "westeurope" + + def test_sites_local_still_wins_over_extras(self, tmp_workspace, tmp_path): + extra = tmp_path / "extra-sites" + extra.mkdir() + _write_site(tmp_workspace / "sites" / "layered.yaml", location="eastus") + _write_site(extra / "layered.yaml", location="westus") + + (tmp_workspace / "sites.local").mkdir() + (tmp_workspace / "sites.local" / "layered.yaml").write_text( + yaml.dump({"location": "northeurope"}) + ) + + orchestrator = Orchestrator(tmp_workspace, extra_trusted_sites_dirs=[extra]) + site = orchestrator.load_site("layered") + assert site.location == "northeurope" + + def test_sites_local_inherits_still_stripped_with_extras_present( + self, tmp_workspace, tmp_path + ): + """The security invariant on sites.local/ must not be weakened by + the presence of extra trusted directories. + """ + extra = tmp_path / "extra-sites" + extra.mkdir() + shared = tmp_path / "shared" + shared.mkdir() + + _write_template(shared / "evil.yaml", subscription="hijacked") + _write_site(tmp_workspace / "sites" / "victim.yaml", subscription="original") + + (tmp_workspace / "sites.local").mkdir() + (tmp_workspace / "sites.local" / "victim.yaml").write_text( + yaml.dump({"inherits": str((shared / "evil.yaml").resolve())}) + ) + + orchestrator = Orchestrator(tmp_workspace, extra_trusted_sites_dirs=[extra]) + site = orchestrator.load_site("victim") + + # inherits on sites.local/ is ignored: base chain stays with sites/. + assert site.subscription == "original" + + def test_site_template_in_extra_dir_excluded_from_listing( + self, tmp_workspace, tmp_path + ): + extra = tmp_path / "extra-sites" + extra.mkdir() + _write_template(extra / "base.yaml", subscription="tmpl") + _write_site(extra / "real.yaml") + + orchestrator = Orchestrator(tmp_workspace, extra_trusted_sites_dirs=[extra]) + names = {s.name for s in orchestrator.load_all_sites()} + assert "real" in names + assert "base" not in names + + +class TestExtraSitesDirsValidation: + """Constructor-time validation of extra_trusted_sites_dirs.""" + + def test_nonexistent_dir_raises(self, tmp_workspace, tmp_path): + missing = tmp_path / "does-not-exist" + with pytest.raises(FileNotFoundError): + Orchestrator(tmp_workspace, extra_trusted_sites_dirs=[missing]) + + def test_file_instead_of_dir_raises(self, tmp_workspace, tmp_path): + f = tmp_path / "just-a-file.txt" + f.write_text("hello") + with pytest.raises(FileNotFoundError): + Orchestrator(tmp_workspace, extra_trusted_sites_dirs=[f]) + + def test_workspace_sites_dir_rejected(self, tmp_workspace): + with pytest.raises(ValueError, match="sites/"): + Orchestrator( + tmp_workspace, + extra_trusted_sites_dirs=[tmp_workspace / "sites"], + ) + + def test_workspace_sites_local_dir_rejected(self, tmp_workspace): + (tmp_workspace / "sites.local").mkdir() + with pytest.raises(ValueError, match="sites.local/"): + Orchestrator( + tmp_workspace, + extra_trusted_sites_dirs=[tmp_workspace / "sites.local"], + ) + + def test_duplicates_deduplicated_preserving_order(self, tmp_workspace, tmp_path): + a = tmp_path / "a" + b = tmp_path / "b" + a.mkdir() + b.mkdir() + # Provide duplicates including a path spelled differently. + orchestrator = Orchestrator( + tmp_workspace, + extra_trusted_sites_dirs=[a, b, a, Path(str(a))], + ) + # Only [a, b] should remain, in that order. + assert orchestrator._extra_trusted_sites_dirs == [a.resolve(), b.resolve()] + + +class TestCliExtraSitesDirsResolution: + """Tests for CLI flag + env var resolution logic.""" + + def test_cli_flag_only(self, tmp_path): + d = tmp_path / "x" + d.mkdir() + with patch.dict(os.environ, {}, clear=False): + os.environ.pop("SITEOPS_EXTRA_SITES_DIRS", None) + assert _resolve_extra_sites_dirs([d]) == [d] + + def test_env_var_only_parsed_with_pathsep(self, tmp_path): + a = tmp_path / "a" + b = tmp_path / "b" + a.mkdir() + b.mkdir() + env_val = os.pathsep.join([str(a), str(b)]) + with patch.dict(os.environ, {"SITEOPS_EXTRA_SITES_DIRS": env_val}): + result = _resolve_extra_sites_dirs(None) + assert result == [a, b] + + def test_env_var_empty_segments_tolerated(self, tmp_path): + a = tmp_path / "a" + a.mkdir() + # Leading / trailing / doubled separators should all be skipped. + env_val = os.pathsep + str(a) + os.pathsep + os.pathsep + with patch.dict(os.environ, {"SITEOPS_EXTRA_SITES_DIRS": env_val}): + assert _resolve_extra_sites_dirs(None) == [a] + + def test_cli_wins_over_env(self, tmp_path, caplog): + cli_dir = tmp_path / "cli" + env_dir = tmp_path / "env" + cli_dir.mkdir() + env_dir.mkdir() + with patch.dict( + os.environ, {"SITEOPS_EXTRA_SITES_DIRS": str(env_dir)} + ), caplog.at_level("INFO", logger="siteops.cli"): + result = _resolve_extra_sites_dirs([cli_dir]) + assert result == [cli_dir] + assert any( + "SITEOPS_EXTRA_SITES_DIRS" in rec.message for rec in caplog.records + ) + + def test_neither_provided_returns_empty(self): + with patch.dict(os.environ, {}, clear=False): + os.environ.pop("SITEOPS_EXTRA_SITES_DIRS", None) + assert _resolve_extra_sites_dirs(None) == []