diff --git a/.rules/python-00.md b/.rules/python-00.md index 93eaf58..11b0bc4 100644 --- a/.rules/python-00.md +++ b/.rules/python-00.md @@ -116,10 +116,12 @@ def login_user(username: str, password: str) -> bool: """Return True if the user is authenticated.""" ... + # login_flow_test.py def test_login_success(): assert login_user("alice", "correct-password") is True + def test_login_failure(): assert not login_user("alice", "wrong-password") ``` diff --git a/.rules/python-context-managers.md b/.rules/python-context-managers.md index bbd5a6e..4d927cd 100644 --- a/.rules/python-context-managers.md +++ b/.rules/python-context-managers.md @@ -23,6 +23,7 @@ Use this for straightforward procedural setup/teardown: ```python from contextlib import contextmanager + @contextmanager def managed_file(path: str, mode: str): f = open(path, mode) @@ -31,6 +32,7 @@ def managed_file(path: str, mode: str): finally: f.close() + # Usage: with managed_file("/tmp/data.txt", "w") as f: f.write("hello") @@ -53,6 +55,7 @@ class Resource: def __exit__(self, exc_type, exc_val, exc_tb): self.conn.close() + # Usage: with Resource() as conn: conn.send("ping") diff --git a/.rules/python-exception-design-raising-handling-and-logging.md b/.rules/python-exception-design-raising-handling-and-logging.md index ce1880a..7554017 100644 --- a/.rules/python-exception-design-raising-handling-and-logging.md +++ b/.rules/python-exception-design-raising-handling-and-logging.md @@ -15,6 +15,7 @@ vendor leakage. class PaymentsError(Exception): """All payment-layer errors.""" + class CardDeclinedError(PaymentsError): # ✅ ends with Error (pep8-naming N818) def __init__(self, code: str, *, retry_after: int | None = None): super().__init__(f"Card declined ({code})") @@ -50,7 +51,9 @@ types, `ValueError` for bad values, and so on), or domain-specific classes. try: token = decode_jwt(payload) except jwt.InvalidTokenError as exc: - raise AuthenticationError("Invalid session token") from exc # ✅ Ruff Tryceratops rule TRY201 + raise AuthenticationError( + "Invalid session token" + ) from exc # ✅ Ruff Tryceratops rule TRY201 ``` When transforming low‑level failures into domain errors, `raise … from …` @@ -115,13 +118,14 @@ duplication and clarifies intent. ```python import logging + logger = logging.getLogger(__name__) # ❌ flake8-logging issues -logging.warning(f"failed for {user_id}") # f-string (flake8-logging LOG004/LOG014) -logging.warning("failed for %s" % user_id) # %-formatting (flake8-logging LOG007) -logging.warn("deprecated") # warn() (flake8-logging LOG009) -logging.error("bad root logger") # root logger usage (flake8-logging LOG015) +logging.warning(f"failed for {user_id}") # f-string (flake8-logging LOG004/LOG014) +logging.warning("failed for %s" % user_id) # %-formatting (flake8-logging LOG007) +logging.warn("deprecated") # warn() (flake8-logging LOG009) +logging.error("bad root logger") # root logger usage (flake8-logging LOG015) # ✅ Correct logger.warning("Failed for user_id=%s", user_id) # lazy interpolation @@ -205,7 +209,7 @@ def charge(amount_pennies: int, card_token: str) -> str: try: return gateway.charge(amount_pennies, card_token) except gateway.Timeout as exc: - raise PaymentsError("Gateway timeout") from exc # ✅ Tryceratops rule TRY201 + raise PaymentsError("Gateway timeout") from exc # ✅ Tryceratops rule TRY201 except gateway.CardDeclined as exc: raise CardDeclinedError(exc.code, retry_after=60) from exc ``` @@ -229,6 +233,7 @@ def must_have_key(d: dict, key: str) -> None: msg = f"Missing required key: {key!r}" raise KeyError(msg) + logger.info("Dispatching order_id=%s to shop_id=%s", order_id, shop_id) # structured ``` diff --git a/.rules/python-generators.md b/.rules/python-generators.md index 1851eea..3a74537 100644 --- a/.rules/python-generators.md +++ b/.rules/python-generators.md @@ -34,6 +34,7 @@ def iter_user_names(users): if user.active and user.name: yield user.name.upper() + def get_names(users): return list(iter_user_names(users)) ``` @@ -50,11 +51,10 @@ def get_names(users): ```python from itertools import islice + def top_active_emails(users): emails = ( - user.email.lower() - for user in users - if user.active and user.email is not None + user.email.lower() for user in users if user.active and user.email is not None ) return list(islice(emails, 10)) ``` diff --git a/.rules/python-pyproject.md b/.rules/python-pyproject.md index f44c16c..a5625d5 100644 --- a/.rules/python-pyproject.md +++ b/.rules/python-pyproject.md @@ -106,7 +106,7 @@ Published "extras" that an *end user* opts into to enable an optional feature of the package, requested with `package[extra]` syntax (for example, `pandas[excel]`). Reach for this only when the extra dependency powers user-facing functionality that not everyone needs — never for development -tooling. Add them with `uv add --optional `: +tooling. Add them with `uv add --optional `: ```toml [project.optional-dependencies] @@ -122,8 +122,9 @@ Tooling only contributors need: test frameworks, linters, type checkers, documentation builders, and property or mutation testers. These are **local-only** — PEP 735 dependency groups are *not* included in published package metadata (they are not part of the wheel), so they must live here rather -than in `project.optional-dependencies`. Add them with `uv add --dev` (the -`dev` group) or `uv add --group `: +than in `project.optional-dependencies`. Add them with +`uv add --dev ` (the `dev` group) or +`uv add --group `: ```toml [dependency-groups] @@ -138,8 +139,9 @@ dev = [ `uv sync` include the `dev` group with no extra flags, so a bare `uv sync` gives a contributor the full toolchain. Adjust this with: -- `--no-dev` or `--no-default-groups` to exclude development dependencies (for - example, when building a wheel or a production install). +- `--no-dev` to exclude only the `dev` group. +- `--no-default-groups` to disable configured default groups while still + permitting explicit selection of other groups. - `--group ` or `--only-group ` to include or isolate a non-default group. - `[tool.uv].default-groups` to change which groups sync by default: @@ -190,19 +192,19 @@ ______________________________________________________________________ ## 5. Declaring a Build System PEP 517/518 require a `[build-system]` table to tell tools how to build and -install your project. A "modern" convention is to specify `setuptools>=61.0` +install your project. A "modern" convention is to specify `setuptools>=64.0` (for editable installs without `setup.py`) or a lighter alternative like `flit_core`. Below is the typical setup using setuptools: ```toml [build-system] -requires = ["setuptools>=61.0", "wheel"] +requires = ["setuptools>=64.0"] build-backend = "setuptools.build_meta" ``` - **`requires`:** A list of packages needed at build time. For editable installs - in `uv`, you need at least `setuptools>=61.0` and `wheel`. (Python - Packaging[^4], Astral Docs[^7]) + in `uv`, `setuptools>=64.0` supplies PEP 660 editable-install support. + (Python Packaging[^4], Astral Docs[^7]) - **`build-backend`:** The entry point for your build backend. `setuptools.build_meta` is the PEP 517-compliant backend for setuptools. (Python Packaging[^4], Astral Docs[^7]) @@ -282,7 +284,7 @@ docs = [ mycli = "my_project.cli:main" [build-system] -requires = ["setuptools>=61.0", "wheel"] +requires = ["setuptools>=64.0"] build-backend = "setuptools.build_meta" [tool.uv] @@ -318,9 +320,8 @@ package = true 4. **Build System:** - - `setuptools>=61.0` plus `wheel` ensures both legacy and editable installs - work. ✱ Newer versions of setuptools support PEP 660 editable installs - without a `setup.py` stub. (Python Packaging[^4], Astral Docs[^7]) + - `setuptools>=64.0` supplies PEP 660 editable-install support without a + `setup.py` stub. (Python Packaging[^4], Astral Docs[^7]) - `build-backend = "setuptools.build_meta"` tells `uv` how to compile your package. (Python Packaging[^4], Astral Docs[^7]) @@ -372,9 +373,9 @@ A "modern" `pyproject.toml` for an Astral `uv` project should: `[dependency-groups]` (the `dev` group installs by default). - Define any CLI or GUI entry points under `[project.scripts]` or `[project.gui-scripts]`. -- Declare a PEP 517 `[build-system]` (e.g. `setuptools>=61.0`, `wheel`, - `setuptools.build_meta`) to support editable installs, or omit it and rely on - `tool.uv.package = true`. +- Declare a PEP 517 `[build-system]` (e.g. `setuptools>=64.0` and + `setuptools.build_meta`) to support editable installs, or omit it and rely + on `tool.uv.package = true`. - Include a `[tool.uv]` section, at minimum `package = true` if you want `uv` to build and install your own package. diff --git a/.rules/python-return.md b/.rules/python-return.md index f4a1262..10c859c 100644 --- a/.rules/python-return.md +++ b/.rules/python-return.md @@ -1,8 +1,8 @@ # flake8-return Style Guide (Python 3.13) -The `flake8-return` rules ensure consistent and explicit return behaviour while -keeping functions clear in intent and free from unnecessary control flow. -Follow these rules: +The `flake8-return` rules ensure consistent and explicit return behaviour. They +keep functions clear in intent and free from unnecessary control flow. Follow +these rules: ## R501 — Avoid Explicit `return None` if It's the Only Return @@ -11,6 +11,7 @@ Follow these rules: def func(): return None + # GOOD: def func(): return @@ -30,6 +31,7 @@ def func(x): return x # implicitly returns None (bad) + # GOOD: def func(x): if x > 0: @@ -41,14 +43,15 @@ Ensure all branches explicitly return a value if any branch does. ______________________________________________________________________ -## R503 — Add an Explicit Return at the End +## R503 — Add an Explicit Return at the End When a Function May Return a Value ```python # BAD: def func(x): if x > 0: return x - # no return (bad) + # missing terminal return (bad) + # GOOD: def func(x): @@ -57,7 +60,17 @@ def func(x): return -1 ``` -Don't rely on implicit `None`—always return something at the end. +Don't rely on implicit `None` if the function may return a value elsewhere—always +return something at the end. + +Functions whose only possible result is `None` do not need a final bare `return`: + +```python +# GOOD: +def func(): + do_something() + # implicit None is fine here +``` ______________________________________________________________________ @@ -69,6 +82,7 @@ def func(): result = compute() return result + # GOOD: def func(): return compute() @@ -114,9 +128,9 @@ for x in xs: log() ``` -These rules apply to regular, and `async def` functions alike. +These rules apply to regular and `async def` functions alike. ______________________________________________________________________ -Use the `flake8-return` rules to enforce predictable return logic. Doing so -enhances readability and correctness. +Use the `flake8-return` rules to enforce predictable and clean return logic, +enhancing readability and correctness. diff --git a/.rules/python-typing.md b/.rules/python-typing.md index 8869023..48b632d 100644 --- a/.rules/python-typing.md +++ b/.rules/python-typing.md @@ -16,14 +16,17 @@ with integers or strings is required (e.g. for database or JSON serialization). ```python import enum + class Status(enum.Enum): PENDING = enum.auto() COMPLETE = enum.auto() + class ErrorCode(enum.IntEnum): OK = 0 NOT_FOUND = 404 + class Role(enum.StrEnum): ADMIN = enum.auto() GUEST = enum.auto() @@ -68,6 +71,7 @@ returns the same instance. ```python import typing + class Builder: def add(self, value: int) -> typing.Self: self.values.append(value) @@ -84,9 +88,10 @@ enables static analysis tools to detect typos and signature mismatches. ```python import typing + class Base: - def run(self) -> None: - ... + def run(self) -> None: ... + class Child(Base): @typing.override @@ -104,6 +109,7 @@ checkers. ```python import typing + def is_str_list(val: list[object]) -> typing.TypeIs[list[str]]: return all(isinstance(x, str) for x in val) ``` @@ -119,6 +125,7 @@ type is provided. ```python T = typing.TypeVar("T", default=int) + class Box[T]: def __init__(self, value: T | None = None): # Fallback to the TypeVar default (int in this example) diff --git a/Makefile b/Makefile index 9620d5c..77df700 100644 --- a/Makefile +++ b/Makefile @@ -9,6 +9,25 @@ UV_ENV = UV_CACHE_DIR=.uv-cache UV_TOOL_DIR=.uv-tools RUFF := $(UV_ENV) uv run ruff TYPOS_VERSION ?= 1.48.0 TYPOS := uv tool run typos@$(TYPOS_VERSION) +# Keep Pylint independent from the project virtual environment. The PyPy shim +# makes the baseline Pylint policy available on every supported host. +PYLINT_PYTHON ?= pypy +PYLINT_TARGETS ?= concordat scripts tests +PYLINT_PYPY_SHIM_REF ?= 726d09f968b4d729ee4b29c71fc732e744854f3b +PYLINT_PYPY_SHIM = git+https://github.com/leynos/pylint-pypy-shim.git@$(PYLINT_PYPY_SHIM_REF) +PYLINT = $(UV_ENV) uv tool run --python $(PYLINT_PYTHON) --from '$(PYLINT_PYPY_SHIM)' pylint-pypy +# Run the df12 plugin in a separate CPython 3.14 process. Keeping its +# dependency out of the PyPy shim avoids interpreter and plugin version skew. +DF12_PYTHON_LINTS_REF ?= 9c835f35b0f1690597ade799c9c6a30bc5922959 +DF12_PYTHON_LINTS = git+https://github.com/leynos/df12-python-lints.git@$(DF12_PYTHON_LINTS_REF) +DF12_PYTHON ?= 3.14 +DF12_PYLINT_TARGETS ?= concordat scripts +DF12_PYLINT_MESSAGES = R9101,C9102,R9103,R9104,C9105,C9106,C9107,R9108,R9109,R9110,R9111,R9112,C9112 +DF12_PYLINT = $(UV_ENV) uv run --isolated --python $(DF12_PYTHON) --with '$(DF12_PYTHON_LINTS)' pylint \ + --disable=all --load-plugins=df12_python_lints --py-version=3.13 \ + --enable=$(DF12_PYLINT_MESSAGES) +AMBRLEAKS = $(UV_ENV) uv tool run --python $(DF12_PYTHON) \ + --from '$(DF12_PYTHON_LINTS)' ambrleaks # Pinned so `make typecheck` reports the same diagnostics locally and in # CI. An unpinned `ty` drifts between machines and hides real findings. TY_VERSION ?= 0.0.65 @@ -74,6 +93,9 @@ check-fmt: build ## Verify formatting lint: build ## Run linters $(RUFF) check + $(PYLINT) $(PYLINT_TARGETS) + $(DF12_PYLINT) $(DF12_PYLINT_TARGETS) + $(AMBRLEAKS) tests +$(MAKE) spelling typecheck: build uv ## Run typechecking @@ -98,7 +120,7 @@ vale: $(VALE) $(ACRONYM_SCRIPT) ## Check prose uv run --with "git+https://github.com/leynos/concordat-vale.git" $(ACRONYM_SCRIPT) $(VALE) --no-global . -test: build uv $(VENV_TOOLS) ## Run tests +test: build spelling uv $(VENV_TOOLS) ## Run tests $(UV_ENV) uv run pytest -v -n auto help: ## Show available targets diff --git a/concordat/__init__.py b/concordat/__init__.py index 8c3c1af..4f171d1 100644 --- a/concordat/__init__.py +++ b/concordat/__init__.py @@ -1,13 +1,5 @@ """concordat package.""" -from __future__ import annotations - -PACKAGE_NAME = "concordat" - -try: # pragma: no cover - Rust optional - rust = __import__(f"_{PACKAGE_NAME}_rs") - hello = rust.hello # type: ignore[attr-defined] -except ModuleNotFoundError: # pragma: no cover - Python fallback - from .pure import hello +from .runtime import hello __all__ = ["hello"] diff --git a/concordat/apply_recovery.py b/concordat/apply_recovery.py index ab530bb..4051b5b 100644 --- a/concordat/apply_recovery.py +++ b/concordat/apply_recovery.py @@ -71,10 +71,7 @@ def _attempt_one_import( repo_name: str, slug: str, ) -> bool: - """Attempt to import a single repository, trying repo_name then slug. - - Returns True if import succeeded, False otherwise. - """ + """Attempt to import a repository using its name, then its slug.""" import_attempts = [repo_name, slug] for import_id in import_attempts: callbacks.write_stream_output( @@ -110,21 +107,7 @@ def _prompt_for_recovery_action( exit_code: int, latest_result: SimpleNamespace, ) -> tuple[bool, int, SimpleNamespace]: - """Prompt user for recovery action approval. - - Args: - prompt_message: The message to display to the user. - non_interactive_message: Message to show when prompting is not possible. - context: Recovery execution context. - callbacks: Recovery callback functions. - exit_code: Current exit code to return if prompting fails. - latest_result: Latest result to return if prompting fails. - - Returns: - Tuple of (should_proceed, exit_code, latest_result). - If should_proceed is False, caller should return (exit_code, latest_result). - - """ + """Prompt for recovery approval and return whether to proceed.""" if not callbacks.can_prompt(): callbacks.write_stream_output(context.io.stderr, non_interactive_message) return False, exit_code, latest_result @@ -173,6 +156,11 @@ def handle_apply_import_errors( Detects GitHub repository existence errors, prompts for import, and retries apply. Returns updated exit code and latest result. + + Returns + ------- + tuple[int, SimpleNamespace] + The final exit code and latest tofu result. """ exit_code = int(latest_result.returncode) @@ -219,16 +207,7 @@ def handle_apply_import_errors( def _line_matches_any_slug(line: str, slugs: list[str]) -> str | None: - """Check if a non-empty line matches any slug pattern. - - Args: - line: A line from tofu state list output. - slugs: List of repository slugs to match against. - - Returns: - The line itself if it matches any slug pattern, None otherwise. - - """ + """Return a non-empty state-list line when it matches a repository slug.""" stripped = line.strip() if not stripped: return None @@ -318,6 +297,11 @@ def handle_apply_prevent_destroy_errors( Detects resources blocked by prevent_destroy, prompts for state removal, and retries apply. Returns updated exit code and latest result. + + Returns + ------- + tuple[int, SimpleNamespace] + The final exit code and latest tofu result. """ exit_code = int(latest_result.returncode) diff --git a/concordat/auditor/checks.py b/concordat/auditor/checks.py index 508bef8..8d7e523 100644 --- a/concordat/auditor/checks.py +++ b/concordat/auditor/checks.py @@ -384,21 +384,17 @@ def _run_permissions(context: AuditContext) -> list[Finding]: resource=resource, ) ) - findings.extend( - [ - Finding( - rule_id="PM-001", - message=( - f"Outside collaborator {collaborator.login} has admin access." - ), - level="error", - resource=resource, - properties={"login": collaborator.login}, - ) - for collaborator in context.collaborators - if collaborator.permissions.get("admin", False) - ] - ) + findings.extend([ + Finding( + rule_id="PM-001", + message=(f"Outside collaborator {collaborator.login} has admin access."), + level="error", + resource=resource, + properties={"login": collaborator.login}, + ) + for collaborator in context.collaborators + if collaborator.permissions.get("admin", False) + ]) return findings diff --git a/concordat/auditor/cli.py b/concordat/auditor/cli.py index eb14bd2..92defcd 100644 --- a/concordat/auditor/cli.py +++ b/concordat/auditor/cli.py @@ -187,11 +187,11 @@ def _repository_from_dict(payload: dict[str, object]) -> RepositorySnapshot: owner=str(payload["owner"]), name=str(payload["name"]), default_branch=str(payload["default_branch"]), - allow_squash_merge=bool(payload.get("allow_squash_merge", False)), - allow_merge_commit=bool(payload.get("allow_merge_commit", False)), - allow_rebase_merge=bool(payload.get("allow_rebase_merge", False)), - allow_auto_merge=bool(payload.get("allow_auto_merge", False)), - delete_branch_on_merge=bool(payload.get("delete_branch_on_merge", False)), + allow_squash_merge=bool(payload.get("allow_squash_merge")), + allow_merge_commit=bool(payload.get("allow_merge_commit")), + allow_rebase_merge=bool(payload.get("allow_rebase_merge")), + allow_auto_merge=bool(payload.get("allow_auto_merge")), + delete_branch_on_merge=bool(payload.get("delete_branch_on_merge")), ) @@ -234,14 +234,14 @@ def _branch_protection_from_dict(payload: dict[str, object]) -> BranchProtection bool(signed_commits_field) if isinstance(signed_commits_field, bool) else None ) return BranchProtection( - enforce_admins=bool(payload.get("enforce_admins", False)), + enforce_admins=bool(payload.get("enforce_admins")), require_signed_commits=signed_commits, - required_linear_history=bool(payload.get("required_linear_history", False)), + required_linear_history=bool(payload.get("required_linear_history")), require_conversation_resolution=bool( - payload.get("require_conversation_resolution", False) + payload.get("require_conversation_resolution") ), - allows_deletions=bool(payload.get("allows_deletions", False)), - allows_force_pushes=bool(payload.get("allows_force_pushes", False)), + allows_deletions=bool(payload.get("allows_deletions")), + allows_force_pushes=bool(payload.get("allows_force_pushes")), status_checks=status_checks, pull_request_reviews=reviews, ) diff --git a/concordat/auditor/github.py b/concordat/auditor/github.py index 8c5b7e3..e1de36c 100644 --- a/concordat/auditor/github.py +++ b/concordat/auditor/github.py @@ -41,13 +41,11 @@ def __init__( self.api_url = api_url.rstrip("/") self.timeout = timeout self.session = requests.Session() - self.session.headers.update( - { - "Authorization": f"Bearer {token}", - "Accept": "application/vnd.github+json", - "User-Agent": "concordat-auditor", - } - ) + self.session.headers.update({ + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "User-Agent": "concordat-auditor", + }) def repository(self, owner: str, name: str) -> RepositorySnapshot: """Return repository metadata used by the repository checks.""" diff --git a/concordat/auditor/models.py b/concordat/auditor/models.py index 9ad1ee9..47111de 100644 --- a/concordat/auditor/models.py +++ b/concordat/auditor/models.py @@ -11,7 +11,7 @@ Severity = str -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class RepositorySnapshot: """Resolved repository settings fetched from the GitHub API.""" @@ -26,11 +26,11 @@ class RepositorySnapshot: @property def slug(self) -> str: - """Return the owner/repo slug for SARIF reporting.""" + """Owner/repo slug for SARIF reporting.""" return f"{self.owner}/{self.name}" -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class RequiredStatusChecks: """Minimal representation of GitHub status check requirements.""" @@ -38,7 +38,7 @@ class RequiredStatusChecks: contexts: tuple[str, ...] -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class RequiredPullRequestReviews: """Review requirements configured for branch protection.""" @@ -47,7 +47,7 @@ class RequiredPullRequestReviews: require_code_owner_reviews: bool -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class BranchProtection: """Branch protection details for the default branch.""" @@ -61,7 +61,7 @@ class BranchProtection: pull_request_reviews: RequiredPullRequestReviews | None -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class TeamPermission: """Team permission assignment exposed by the GitHub API.""" @@ -69,7 +69,7 @@ class TeamPermission: permission: str -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class CollaboratorPermission: """Direct collaborator permission assignment.""" @@ -78,7 +78,7 @@ class CollaboratorPermission: permissions: dict[str, bool] -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class LabelState: """GitHub label attributes relevant to Concordat.""" @@ -87,7 +87,7 @@ class LabelState: description: str -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class AuditContext: """Aggregated context handed to each check.""" @@ -99,7 +99,7 @@ class AuditContext: priority_model: PriorityModel | None -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class CheckDefinition: """Metadata describing a Concordat Auditor rule.""" @@ -111,7 +111,7 @@ class CheckDefinition: help_uri: str | None = None -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class Finding: """Single SARIF finding emitted by a check.""" diff --git a/concordat/auditor/priority.py b/concordat/auditor/priority.py index 7c632ca..a41c294 100644 --- a/concordat/auditor/priority.py +++ b/concordat/auditor/priority.py @@ -11,7 +11,7 @@ yaml = YAML(typ="safe") -@dc.dataclass(frozen=True) +@dc.dataclass(frozen=True, slots=True) class PriorityLabel: """Single canonical label entry.""" @@ -21,7 +21,7 @@ class PriorityLabel: description: str -@dc.dataclass(frozen=True) +@dc.dataclass(frozen=True, slots=True) class PriorityFieldOption: """Projects v2 field option derived from the canonical model.""" @@ -29,7 +29,7 @@ class PriorityFieldOption: display_name: str -@dc.dataclass(frozen=True) +@dc.dataclass(frozen=True, slots=True) class PriorityField: """Projects v2 field contract.""" @@ -38,7 +38,7 @@ class PriorityField: options: tuple[PriorityFieldOption, ...] -@dc.dataclass(frozen=True) +@dc.dataclass(frozen=True, slots=True) class PriorityModel: """Top-level model consumed by the Auditor.""" @@ -105,7 +105,7 @@ def load_priority_model(path: pathlib.Path | None) -> PriorityModel: if not target.exists(): return _DEFAULT_MODEL - data = yaml.load(target.read_text()) + data = yaml.load(target.read_text(encoding="utf-8")) try: schema_version = int(data["schema_version"]) except (KeyError, TypeError, ValueError) as error: diff --git a/concordat/canon_artifacts.py b/concordat/canon_artifacts.py index a31d69f..1685690 100644 --- a/concordat/canon_artifacts.py +++ b/concordat/canon_artifacts.py @@ -5,8 +5,6 @@ against a checked-out (published) platform-standards repository. """ -# ruff: noqa: TRY003 - from __future__ import annotations import dataclasses @@ -75,7 +73,13 @@ class CanonManifest: @property def template_root(self) -> Path: - """Return the Concordat checkout root containing the manifest.""" + """Concordat checkout root containing the manifest. + + Returns + ------- + Path + Concordat checkout root containing the manifest. + """ return self.manifest_path.parent.parent.parent @@ -138,7 +142,7 @@ def resolve_concordat_root(start: Path | None = None) -> Path: for candidate in (cursor, *cursor.parents): if (candidate / DEFAULT_MANIFEST_RELATIVE).exists(): return candidate - raise CanonArtifactsError( + raise CanonArtifactsError( # noqa: TRY003 # Domain error provides operator remediation. "Unable to locate platform-standards template tree. " f"Expected to find {DEFAULT_MANIFEST_RELATIVE} in a parent directory; " "pass --template-root explicitly." @@ -150,7 +154,7 @@ def _validate_manifest_structure( ) -> dict[str, object]: """Validate basic manifest structure.""" if not isinstance(data, dict): - raise CanonArtifactsError( + raise CanonArtifactsError( # noqa: TRY003 # Domain error identifies the invalid manifest. f"Manifest content must be a mapping: {manifest_path}" ) return typ.cast("dict[str, object]", data) @@ -160,7 +164,7 @@ def _validate_schema_version(data: dict[str, object], manifest_path: Path) -> in """Validate and return schema version.""" schema_version = data.get("schema_version") if schema_version != 1: - raise CanonArtifactsError( + raise CanonArtifactsError( # noqa: TRY003 # Domain error explains the unsupported input. f"Unsupported manifest schema_version={schema_version!r} " f"(expected 1): {manifest_path}" ) @@ -181,7 +185,7 @@ def _validate_artifacts_list( ) -> list[object]: """Validate and return the artifacts list.""" if not isinstance(artifacts_raw, list) or not artifacts_raw: - raise CanonArtifactsError( + raise CanonArtifactsError( # noqa: TRY003 # Domain error identifies the invalid manifest. f"Manifest artifacts must be a non-empty list: {manifest_path}" ) return typ.cast("list[object]", artifacts_raw) @@ -190,7 +194,7 @@ def _validate_artifacts_list( def _parse_single_artifact(entry: object, manifest_path: Path) -> CanonArtifact: """Parse a single artifact mapping into a CanonArtifact.""" if not isinstance(entry, dict): - raise CanonArtifactsError( + raise CanonArtifactsError( # noqa: TRY003 # Domain error identifies the invalid manifest. f"Manifest artifact entries must be mappings: {manifest_path}" ) @@ -204,7 +208,7 @@ def _parse_single_artifact(entry: object, manifest_path: Path) -> CanonArtifact: sha256=str(entry_map["sha256"]), ) except KeyError as exc: - raise CanonArtifactsError( + raise CanonArtifactsError( # noqa: TRY003 # Domain error identifies the invalid manifest. f"Manifest artifact missing key {exc.args[0]!r}: {manifest_path}" ) from exc @@ -212,7 +216,9 @@ def _parse_single_artifact(entry: object, manifest_path: Path) -> CanonArtifact: def load_manifest(manifest_path: Path) -> CanonManifest: """Load and validate the canonical artifact manifest.""" if not manifest_path.exists(): - raise CanonArtifactsError(f"Manifest not found: {manifest_path}") + raise CanonArtifactsError( # noqa: TRY003 # Domain error identifies the missing manifest. + f"Manifest not found: {manifest_path}" + ) data = _yaml.load(manifest_path.read_text(encoding="utf-8")) manifest_data = _validate_manifest_structure(data, manifest_path) schema_version = _validate_schema_version(manifest_data, manifest_path) @@ -229,7 +235,9 @@ def sha256_digest(path: Path) -> str: if path.is_file(): return hashlib.sha256(path.read_bytes()).hexdigest() if not path.is_dir(): - raise CanonArtifactsError(f"Expected file or directory, got: {path}") + raise CanonArtifactsError( # noqa: TRY003 # Domain error identifies the invalid path. + f"Expected file or directory, got: {path}" + ) hasher = hashlib.sha256() for file_path in sorted(p for p in path.rglob("*") if p.is_file()): @@ -334,6 +342,11 @@ def sync_artifacts( comparisons: The artifact comparisons to consider for syncing. config: Sync configuration (roots, filters, and dry-run behaviour). + Returns + ------- + tuple[SyncAction, ...] + Actions performed or planned for the selected artifacts. + """ actions: list[SyncAction] = [] for comparison in comparisons: diff --git a/concordat/cli.py b/concordat/cli.py index 9cec43c..c9d736a 100644 --- a/concordat/cli.py +++ b/concordat/cli.py @@ -62,7 +62,7 @@ ) ERROR_NO_ESTATES = "No estates configured. Run `concordat estate init` first." ERROR_MISSING_GITHUB_TOKEN = ( - "GITHUB_TOKEN is required for concordat plan/apply; " # noqa: S105 + "GITHUB_TOKEN is required for concordat plan/apply; " # noqa: S105 # Error text only; no secret value. "pass --github-token or export the environment variable." ) ERROR_AUTO_APPROVE_REQUIRED = "concordat apply requires --auto-approve to continue." @@ -335,6 +335,12 @@ def rule_run( Exit codes: 0 compliant; 1 at least one finding (including indeterminate verdicts, which fail closed); 2 operational failure. + + Returns + ------- + int + Exit code for the audit result. + """ from .rules import render_json, render_table, run_rule diff --git a/concordat/credentials.py b/concordat/credentials.py index 8b2d006..28f0e64 100644 --- a/concordat/credentials.py +++ b/concordat/credentials.py @@ -122,6 +122,12 @@ def credential_environment( Environment variables always win; file values only fill gaps. When *owner* is omitted the headline active owner scopes the file; with no resolvable owner the environment passes through unchanged. + + Returns + ------- + dict[str, str] + Environment values with file-backed credentials filling missing keys. + """ source = _environ(env) merged = dict(source) diff --git a/concordat/enrol.py b/concordat/enrol.py index 1d705c9..e0269f9 100644 --- a/concordat/enrol.py +++ b/concordat/enrol.py @@ -116,13 +116,7 @@ def _owner_mismatch_error( def _render_platform_pr_result(result: PlatformStandardsResult) -> str: - """Render the platform inventory PR outcome. - - The active estate inventory (and therefore `concordat estate show`) reflects - the estate repository default branch. When concordat opens/updates a feature - branch PR in platform-standards, the repository will not appear in the - estate inventory until the PR is merged. - """ + """Render the platform inventory PR outcome and estate-update implications.""" if result.created: message = "platform PR opened" if result.pr_url: @@ -138,18 +132,7 @@ def _build_status_parts( pushed: bool = False, platform_pr: PlatformStandardsResult | None = None, ) -> list[str]: - """Build list of status message parts from base message and optional flags. - - Args: - base_message: The initial status message fragment. - committed: Whether to append "committed" to the parts. - pushed: Whether to append "pushed" to the parts. - platform_pr: Optional platform PR result to append. - - Returns: - List of status message fragments ready for joining. - - """ + """Build status message parts from a base message and optional flags.""" parts = [base_message] if committed: parts.append("committed") @@ -161,21 +144,12 @@ def _build_status_parts( def _format_outcome(repository: str, status_parts: list[str]) -> str: - """Format repository and status parts into a final outcome message. - - Args: - repository: The repository specification. - status_parts: List of status message fragments. - - Returns: - Formatted message: "{repository}: {joined parts}" - - """ + """Format repository and status parts into a final outcome message.""" status = ", ".join(status_parts) return f"{repository}: {status}" -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class EnrollmentOutcome: """Captured outcome for a processed repository.""" @@ -198,7 +172,7 @@ def render(self) -> str: return _format_outcome(self.repository, status_parts) -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class DisenrollmentOutcome: """Captured outcome for a processed repository during disenrolment.""" @@ -437,17 +411,7 @@ def _execute_platform_pr_operation( platform_standards: PlatformStandardsConfig | None, operation: typ.Callable[[str, PlatformStandardsConfig], PlatformStandardsResult], ) -> PlatformStandardsResult | None: - """Execute a platform PR operation with common validation and error handling. - - Args: - repo_slug: GitHub repository slug (owner/repo) or None. - platform_standards: Platform configuration or None. - operation: The platform operation to execute (e.g., ensure_repository_pr). - - Returns: - PlatformStandardsResult if operation was attempted, None if config missing. - - """ + """Execute a platform PR operation with validation and error handling.""" if platform_standards is None: return None if not repo_slug: @@ -492,7 +456,7 @@ def _platform_pr_removal_result( ) -@dataclasses.dataclass +@dataclasses.dataclass(slots=True) class _RepositoryContext: repository: Repository location: Path diff --git a/concordat/estate.py b/concordat/estate.py index e2f0076..30682a5 100644 --- a/concordat/estate.py +++ b/concordat/estate.py @@ -218,16 +218,7 @@ def _resolve_implicit_config_path( config_path: Path | None, estate_owner: str, ) -> tuple[Path | None, str | None]: - """Resolve the estate config path and the owner to activate on success. - - The duplicate-alias check and the eventual registration must read and - write the same owner-namespaced file, so the path is resolved up front and - any active-owner mismatch is rejected here. The active owner is NOT mutated: - the returned ``owner_to_activate`` (non-``None`` only on the implicit path - with no active owner yet) is committed by the caller after registration - succeeds, so a failed init leaves ``xdg.get_active_owner()`` unchanged. An - explicit *config_path* bypasses the owner namespace entirely. - """ + """Resolve the estate config path and owner to activate on success.""" match (config_path, xdg.get_active_owner()): case (Path() as explicit, _): return explicit, None diff --git a/concordat/estate_cache.py b/concordat/estate_cache.py index e85209a..671586a 100644 --- a/concordat/estate_cache.py +++ b/concordat/estate_cache.py @@ -44,6 +44,17 @@ def cache_destination( An explicit *cache_directory* bypasses namespacing (the test seam). Otherwise the record's ``github_owner`` — or the headline active owner — selects ``$XDG_CACHE_HOME/concordat/owners//estates``. + + Returns + ------- + Path + Cache directory path for the estate. + + Raises + ------ + EstateCacheError + If no owner is available for cache namespacing. + """ if cache_directory is not None: return cache_directory / record.alias @@ -127,6 +138,12 @@ def clone_into_temp( With *runs_root* the copy lands in a unique subdirectory of that directory (the owner's XDG state runs area); otherwise it falls back to the system temporary directory. + + Returns + ------- + Path + Path to the isolated working directory. + """ if runs_root is not None: runs_root.mkdir(parents=True, exist_ok=True) diff --git a/concordat/estate_config.py b/concordat/estate_config.py index 469d872..8bfcdd9 100644 --- a/concordat/estate_config.py +++ b/concordat/estate_config.py @@ -76,6 +76,12 @@ def default_config_path() -> Path: no active owner the flat path is returned. Any legacy-flat migration is a separate, explicit bootstrap step (see :func:`migrate_legacy_config`), run once at the CLI entry point rather than implicitly from this getter. + + Returns + ------- + Path + Active-owner configuration path, or the legacy flat path when no owner + is active. """ if owner := xdg.get_active_owner(): return xdg.owner_config_path(owner) @@ -85,13 +91,7 @@ def default_config_path() -> Path: def _load_legacy_migration() -> ( tuple[Path, dict[str, typ.Any], dict[str, typ.Any], str] | None ): - """Return the legacy migration inputs, or ``None`` when none applies. - - The tuple is ``(legacy, full_data, estate_section, owner)``. ``None`` is - returned when an active owner is already configured, the flat config is - absent, the parsed YAML or estate section is not a mapping, or no owner can - be derived from the recorded estates. - """ + """Return legacy migration inputs, or ``None`` when none applies.""" if xdg.get_active_owner() is not None: return None legacy = xdg.config_root() / CONFIG_FILENAME @@ -166,15 +166,7 @@ def _current_legacy_data( legacy: Path, fallback: dict[str, typ.Any], ) -> dict[str, typ.Any]: - """Return the legacy file's contents as they stand now. - - The legacy flat config and the XDG headline config are the same file, so - :func:`xdg.set_active_owner` writes the active-owner key into it. Cleanup - must therefore drop the estate section from what is on disk *after* that - write; rewriting the snapshot taken before it would erase the key, or - delete the file outright when the estate section was its only content, - leaving the migrated estates unreachable. - """ + """Return the legacy file's current contents, or *fallback* if absent.""" if not legacy.is_file(): return fallback try: @@ -218,13 +210,7 @@ def migrate_legacy_config() -> None: def _derive_owner_from_estates(estate_section: dict[str, typ.Any]) -> str | None: - """Return the sole github_owner recorded in a legacy estate section. - - The legacy flat format permitted estates for more than one owner. Moving - such a section wholesale into a single owner's namespace would silently - misplace the other owners' estates, so mixed-owner input is rejected - rather than migrated under the first owner encountered. - """ + """Return the sole GitHub owner recorded in a legacy estate section.""" estates = estate_section.get(ESTATE_COLLECTION_KEY) if not isinstance(estates, dict): return None @@ -334,13 +320,7 @@ def _estate_record_from_payload( alias: str, payload: object, ) -> EstateRecord | None: - """Decode one persisted estate payload, or reject an unsupported one. - - A bare string is the legacy shorthand for a repository URL. A mapping is - accepted only when it carries a string ``repo_url``; anything else — a - list, a scalar, or a mapping without a usable URL — is rejected so the - caller can skip it. - """ + """Decode one persisted estate payload, or reject an unsupported one.""" match payload: case str(): return EstateRecord(alias=alias, repo_url=payload) diff --git a/concordat/estate_errors.py b/concordat/estate_errors.py index eda3593..8ee1f1e 100644 --- a/concordat/estate_errors.py +++ b/concordat/estate_errors.py @@ -235,10 +235,10 @@ def __init__(self, active_owner: str, estate_owner: str) -> None: class GitHubOrganizationAuthenticationError(GitHubAuthenticationError): - """Raised when organisation-level auth fails.""" + """Raised when organization-level auth fails.""" def __init__(self, owner: str) -> None: - """Initialise the error with the organisation owner.""" + """Initialise the error with the organization owner.""" message = ( f"GitHub authentication failed accessing organization {owner!r}. " "Ensure GITHUB_TOKEN includes the 'repo' scope and is valid." diff --git a/concordat/estate_execution.py b/concordat/estate_execution.py index ca9f75c..b71499d 100644 --- a/concordat/estate_execution.py +++ b/concordat/estate_execution.py @@ -20,7 +20,9 @@ from .estate_cache import ( EstateCacheError, clone_into_temp, - ensure_estate_cache, +) +from .estate_cache import ( + ensure_estate_cache as _ensure_estate_cache_original, ) from .tofu_github_errors import ( detect_missing_repo_imports as _detect_missing_repo_imports, @@ -96,8 +98,6 @@ class EstateExecutionError(ConcordatError): # Make EstateCacheError raise as EstateExecutionError for backward compatibility. -# Store original function reference before wrapping. -_ensure_estate_cache_original = ensure_estate_cache def _wrap_cache_error[T, **P](func: typ.Callable[P, T]) -> typ.Callable[P, T]: @@ -125,7 +125,7 @@ def _resolve_backend_environment(env: typ.Mapping[str, str]) -> dict[str, str]: raise EstateExecutionError(str(error)) from error -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class ExecutionOptions: """User-configurable knobs for running tofu against an estate.""" @@ -137,7 +137,7 @@ class ExecutionOptions: environment: cabc.Mapping[str, str] | None = None -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class ExecutionIO: """Output streams used by the tofu runner.""" @@ -145,7 +145,7 @@ class ExecutionIO: stderr: typ.IO[str] -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class WorkspaceContext: """Workspace directory paths for tofu operations.""" @@ -153,7 +153,7 @@ class WorkspaceContext: tofu_dir: Path -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class ExecutionContext: """Execution environment for tofu commands.""" @@ -162,7 +162,7 @@ class ExecutionContext: env: dict[str, str] -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class PersistenceRuntime: """Runtime data for invoking tofu with a remote backend.""" @@ -193,6 +193,11 @@ def estate_workspace( *cache_directory* seam: injecting a cache directory bypasses the owner-required cache resolution, so an ownerless record can get that far and land in the system temporary directory instead. + + Yields + ------ + Path + Isolated working-tree path for the estate operation. """ cache_path = ensure_estate_cache(record, cache_directory=cache_directory) runs_root: Path | None = None @@ -266,6 +271,11 @@ def _prepare_execution_environment(options: ExecutionOptions) -> dict[str, str]: credential fallbacks (environment variables win), and the shared OpenTofu provider plugin cache is enabled unless the caller already set one. + + Returns + ------- + dict[str, str] + Environment mapping prepared for OpenTofu invocation. """ env_source = _credentials.credential_environment( owner=options.github_owner or None, @@ -287,8 +297,13 @@ def _setup_tofu_workspace( ) -> tuple[list[str], Tofu]: """Prepare the workspace for tofu execution. - Sanitizes inventory, writes tfvars, configures backend, and initialises the + Sanitizes inventory, writes tfvars, configures backend, and initializes the tofu wrapper. Returns backend arguments and tofu instance. + + Returns + ------- + tuple[list[str], Tofu] + Backend command-line arguments and the initialized tofu wrapper. """ sanitized_inventory = _sanitize_inventory_for_tofu( workspace.root, @@ -328,6 +343,11 @@ def _execute_apply_command( Runs apply, then handles import and prevent_destroy errors if they occur. Returns the final exit code. + + Returns + ------- + int + Final OpenTofu apply exit code after recovery handling. """ result = invoke_tofu_command_with_result(tofu, list(args), io) exit_code = int(result.returncode) diff --git a/concordat/estate_git.py b/concordat/estate_git.py index b3ea309..618ee7a 100644 --- a/concordat/estate_git.py +++ b/concordat/estate_git.py @@ -56,14 +56,7 @@ def default_template_root() -> Path: def _probe_remote(repo_url: str) -> RemoteProbe: - """Report whether *repo_url* can be listed, and whether it holds any refs. - - A remote this cannot reach is reported as ``exists=False`` as well as - ``reachable=False``, because listing is the only evidence available here: - an unroutable host and an absent repository both surface as one - ``GitError``. Callers therefore route an unreachable remote down the - missing-remote path, which asks GitHub what is actually there. - """ + """Report whether *repo_url* can be listed and whether it holds refs.""" callbacks = build_remote_callbacks(repo_url) with TemporaryDirectory(prefix="concordat-estate-probe-") as temp_root: repository = pygit2.init_repository(temp_root) @@ -100,11 +93,7 @@ def _collect_inventory(record: EstateRecord) -> list[str]: def _inventory_slugs(entries: cabc.Iterable[object]) -> cabc.Iterator[str]: - """Yield the trimmed name of each usable entry, skipping the rest. - - An entry is usable only when it is mapping-shaped and carries a ``name`` - that is a non-blank string; anything else is inventory noise. - """ + """Yield the trimmed name of each usable inventory entry.""" for entry in entries: match entry: case {"name": str() as name} if name.strip(): @@ -147,12 +136,7 @@ def _bootstrap_template( repo_url: str, bootstrap: TemplateBootstrap, ) -> None: - """Seed *repo_url* from the bundled template as one atomic operation. - - Validates template availability, copies and sanitizes the template, - initialises and commits a Git repository, pushes the target branch, and - sets the local remote HEAD where applicable. - """ + """Seed *repo_url* from the bundled template as one atomic operation.""" branch = bootstrap.branch if not bootstrap.template_root.exists(): raise TemplateMissingError(bootstrap.template_root) diff --git a/concordat/estate_github.py b/concordat/estate_github.py index 06dd1ef..34fea15 100644 --- a/concordat/estate_github.py +++ b/concordat/estate_github.py @@ -33,13 +33,11 @@ # Both creation paths must offer the same repository, so the options they share # are declared once here and unpacked at each call site, leaving only the name # path-specific. The proxy keeps the shared mapping immutable. -_REPOSITORY_OPTIONS: cabc.Mapping[str, object] = types.MappingProxyType( - { - "private": True, - "auto_init": False, - "description": "Platform standards repository managed by concordat", - } -) +_REPOSITORY_OPTIONS: cabc.Mapping[str, object] = types.MappingProxyType({ + "private": True, + "auto_init": False, + "description": "Platform standards repository managed by concordat", +}) # github3 raises a 401 as `AuthenticationFailed` and a 403 as `ForbiddenError`; # they are siblings rather than one deriving from the other, so both must be @@ -72,7 +70,7 @@ def _create_repository( owner: str, name: str, ) -> None: - """Create a repository in an organisation or for the authenticated user.""" + """Create a repository in an organization or for the authenticated user.""" org = _find_organization(client, owner) if org: _create_organization_repository(org, owner, name) @@ -84,11 +82,21 @@ def _find_organization( client: github3.GitHub, owner: str, ) -> github3.orgs.Organization | None: - """Return the organisation named *owner*, or ``None`` when there is none. + """Return the organization named *owner*, or ``None`` when there is none. An authentication failure here precedes any creation attempt: a rejected - lookup says nothing about whether *owner* is an organisation, so falling + lookup says nothing about whether *owner* is an organization, so falling through to the personal path would misreport the cause. + + Returns + ------- + github3.orgs.Organization | None + Matching organization, or ``None`` when *owner* is not an organization. + + Raises + ------ + GitHubOrganizationAuthenticationError + If GitHub rejects the organization lookup. """ try: return client.organization(owner) @@ -103,7 +111,7 @@ def _create_organization_repository( owner: str, name: str, ) -> None: - """Create *name* inside the *owner* organisation.""" + """Create *name* inside the *owner* organization.""" try: org.create_repository(name, **_REPOSITORY_OPTIONS) except _REJECTED as error: diff --git a/concordat/estate_repository.py b/concordat/estate_repository.py index ca15686..fd99cc9 100644 --- a/concordat/estate_repository.py +++ b/concordat/estate_repository.py @@ -145,11 +145,7 @@ def _plan_missing_repository(slug: str | None) -> RepositoryPlan: def _plan_reachable_repository(repo_url: str, probe: RemoteProbe) -> RepositoryPlan: - """Plan for a reachable, existing remote; only empty ones are usable. - - Callers must route only reachable probes here, so no reachability check is - repeated. A non-empty remote is rejected without contacting GitHub. - """ + """Plan use of a reachable, existing remote when it is empty.""" if not probe.empty: raise NonEmptyRepositoryError(repo_url) return RepositoryPlan( @@ -167,12 +163,7 @@ def _plan_unreachable_repository( github_token: str | None, client_factory: typ.Callable[[str | None], github3.GitHub] | None, ) -> RepositoryPlan: - """Plan for an unreachable remote by asking GitHub whether it exists. - - A repository GitHub can see but SSH cannot reach is inaccessible rather - than missing. The constructed client is returned in the plan so - ``_ensure_repository_exists`` reuses it instead of authenticating twice. - """ + """Plan an unreachable remote by asking GitHub whether it exists.""" if not slug: raise RepositoryUnreachableError(repo_url) # Identity first, matching `_ensure_repository_exists`: a malformed slug @@ -196,15 +187,7 @@ def _lookup_repository( owner: str, name: str, ) -> object | None: - """Return the GitHub repository *owner*/*name*, or ``None`` when absent. - - ``GitHub.repository`` signals an absent repository by raising rather than - returning ``None``, so a ``NotFoundError`` is the answer "it is not there" - and must not escape. Every other GitHub failure leaves the remote's state - unknown, so it is translated rather than surfaced raw: a rejected call is - an authentication problem, and anything else means the lookup itself could - not be completed. - """ + """Return the GitHub repository *owner*/*name*, or ``None`` when absent.""" try: return client.repository(owner, name) except github3_exceptions.NotFoundError: diff --git a/concordat/listing.py b/concordat/listing.py index fcd2db7..d963640 100644 --- a/concordat/listing.py +++ b/concordat/listing.py @@ -120,5 +120,4 @@ def _fetch_namespace(client: _RepositoryClient, namespace: str) -> list[str]: raise _connection_error(error) from error except GitHubError as error: raise _github_api_error(error) from error - else: - return ssh_urls + return ssh_urls diff --git a/concordat/persistence/backend.py b/concordat/persistence/backend.py index 6822f01..46e1f77 100644 --- a/concordat/persistence/backend.py +++ b/concordat/persistence/backend.py @@ -9,7 +9,7 @@ import os import typing as typ -from pathlib import Path # noqa: TC003 +from pathlib import Path # noqa: TC003 # Runtime annotations require this import. from concordat.errors import ConcordatError from concordat.persistence import models as persistence_models @@ -24,7 +24,7 @@ "SPACES_ACCESS_KEY_ID", "SPACES_SECRET_ACCESS_KEY", ) -AWS_SESSION_TOKEN_VAR = "AWS_SESSION_TOKEN" # noqa: S105 +AWS_SESSION_TOKEN_VAR = "AWS_SESSION_TOKEN" # noqa: S105 # Environment-variable name, not a credential. # All backend environment variables for iteration. ALL_BACKEND_ENV_VARS = ( @@ -55,7 +55,9 @@ def session_token_overrides(env: typ.Mapping[str, str]) -> dict[str, str]: Args: env: Environment mapping to check for session token. - Returns: + Returns + ------- + dict[str, str] Dict with AWS_SESSION_TOKEN if present and non-empty, empty dict otherwise. """ @@ -90,11 +92,15 @@ def resolve_backend_environment(env: typ.Mapping[str, str]) -> dict[str, str]: Args: env: Environment mapping to search for credentials. - Returns: + Returns + ------- + dict[str, str] Dict with AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY set. - Raises: - BackendConfigurationError: If no valid credentials are found. + Raises + ------ + BackendConfigurationError + If no valid credentials are found. """ @@ -132,11 +138,15 @@ def validate_backend_path(workdir: Path, backend_config_path: str) -> Path: workdir: The workspace root directory. backend_config_path: Relative path to the backend config file. - Returns: + Returns + ------- + Path The validated relative path to the backend config file. - Raises: - BackendConfigurationError: If the path escapes the workspace or is missing. + Raises + ------ + BackendConfigurationError + If the path escapes the workspace or is missing. """ backend_path = (workdir / backend_config_path).resolve() @@ -161,7 +171,9 @@ def build_object_key(descriptor: PersistenceDescriptor) -> str: Args: descriptor: Persistence descriptor with key_prefix and key_suffix. - Returns: + Returns + ------- + str The full S3 object key path. """ @@ -187,13 +199,21 @@ def get_persistence_runtime( tofu_workdir: Directory containing tofu configuration. env: Environment mapping for credential resolution. - Returns: + Returns + ------- + tuple[ + persistence_models.PersistenceDescriptor | None, + str | None, + str | None, + dict[str, str] | None, + ] A tuple of (descriptor, backend_config, object_key, env_overrides). Returns (None, None, None, None) if persistence is disabled or missing. - Raises: - BackendConfigurationError: If the manifest is invalid or credentials - are missing. + Raises + ------ + BackendConfigurationError + If the manifest is invalid or credentials are missing. """ manifest_path = workspace_root / persistence_models.MANIFEST_FILENAME diff --git a/concordat/persistence/endpoints.py b/concordat/persistence/endpoints.py index 6a61769..44fe50e 100644 --- a/concordat/persistence/endpoints.py +++ b/concordat/persistence/endpoints.py @@ -9,6 +9,13 @@ def normalize_endpoint_url(endpoint: str, *, default_scheme: str = "https") -> s Persistence endpoints are typically supplied as hostnames, but boto3 expects fully qualified URLs (including scheme). When the user omits a scheme, we assume HTTPS by default. + + Returns + ------- + str + Endpoint URL with an explicit scheme, or an empty string for blank + input. + """ cleaned = endpoint.strip() if not cleaned: diff --git a/concordat/persistence/files.py b/concordat/persistence/files.py index 21145bc..e935b8f 100644 --- a/concordat/persistence/files.py +++ b/concordat/persistence/files.py @@ -1,5 +1,4 @@ """File persistence helpers for backend and manifest artifacts.""" -# ruff: noqa: TRY003 from __future__ import annotations @@ -97,13 +96,11 @@ def _enforce_existing_policy( is_same: bool, force: bool, ) -> bool: - """Return True if caller should write, False if identical, else raise. - - When ``is_same`` is False and ``force`` is False, raises a PersistenceError - to protect existing files from accidental overwrite. - """ + """Enforce the existing-file policy, raising on an unsafe replacement.""" if is_same: return False if not force: - raise PersistenceError(f"{path} already exists; rerun with --force to replace.") + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. + f"{path} already exists; rerun with --force to replace." + ) return True diff --git a/concordat/persistence/gitops.py b/concordat/persistence/gitops.py index 826c0df..02e86a1 100644 --- a/concordat/persistence/gitops.py +++ b/concordat/persistence/gitops.py @@ -1,5 +1,4 @@ """Git operations for persistence workflow.""" -# ruff: noqa: TRY003 from __future__ import annotations @@ -28,12 +27,12 @@ def _verify_checkout_succeeded(repository: pygit2.Repository, branch_name: str) try: current = repository.head.shorthand except (KeyError, ValueError, pygit2.GitError) as exc: - raise PersistenceError( + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. f"Failed to confirm checkout away from {branch_name!r}." ) from exc if current == branch_name: - raise PersistenceError( + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. f"Failed to leave branch {branch_name!r} before recreation." ) @@ -50,7 +49,7 @@ def _ensure_not_on_branch( try: repository.checkout(f"refs/heads/{base_branch}") except pygit2.GitError as exc: - raise PersistenceError( + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. f"Unable to checkout base branch {base_branch!r} before " f"recreating {branch_name!r}." ) from exc @@ -70,7 +69,7 @@ def _recreate_branch_if_exists( try: repository.branches.delete(branch_name) except pygit2.GitError as exc: - raise PersistenceError( + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. f"Unable to delete existing branch {branch_name!r}." ) from exc @@ -135,7 +134,9 @@ def _resolve_remote(repository: pygit2.Repository, repo_url: str) -> pygit2.Remo """Select a remote matching repo_url, falling back to origin or the first remote.""" remotes = list(repository.remotes) if not remotes: - raise PersistenceError("Repository has no remotes configured for persistence.") + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. + "Repository has no remotes configured for persistence." + ) for remote in remotes: if _urls_match(remote.url, repo_url): diff --git a/concordat/persistence/inputs.py b/concordat/persistence/inputs.py index 50a7de1..007c0b0 100644 --- a/concordat/persistence/inputs.py +++ b/concordat/persistence/inputs.py @@ -1,5 +1,4 @@ """User input collection and descriptor construction.""" -# ruff: noqa: TRY003 from __future__ import annotations @@ -89,7 +88,7 @@ def _collect_single_input( if default: return default - raise PersistenceError( + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. f"{label} is required in non-interactive mode; provide a flag or " "environment variable." ) @@ -106,7 +105,9 @@ def _prompt_with_default( return response if default: return default - raise PersistenceError(f"{label} is required.") + raise PersistenceError( # noqa: TRY003 # Domain error identifies required input. + f"{label} is required." + ) def _build_descriptor( diff --git a/concordat/persistence/models.py b/concordat/persistence/models.py index 1490ee9..3866b66 100644 --- a/concordat/persistence/models.py +++ b/concordat/persistence/models.py @@ -1,5 +1,4 @@ """Data structures and shared constants for persistence workflow.""" -# ruff: noqa: TRY003 from __future__ import annotations @@ -59,7 +58,7 @@ def delete_object(self, **kwargs: object) -> dict[str, typ.Any]: """Delete an object from the bucket.""" -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class PersistenceDescriptor: """Machine-readable manifest describing the remote state backend.""" @@ -80,10 +79,12 @@ def from_yaml(cls, path: Path) -> PersistenceDescriptor | None: return None loaded = _yaml.load(path.read_text(encoding="utf-8")) or {} if not isinstance(loaded, dict): - raise PersistenceError(f"Invalid persistence manifest at {path}") + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. + f"Invalid persistence manifest at {path}" + ) schema_version = int(loaded.get("schema_version", 0)) if schema_version > PERSISTENCE_SCHEMA_VERSION: - raise PersistenceError( + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. "Unsupported persistence manifest " f"schema_version={schema_version} at {path}; maximum supported " f"schema_version is {PERSISTENCE_SCHEMA_VERSION}" @@ -130,7 +131,7 @@ def to_dict(self) -> dict[str, typ.Any]: return payload -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class PersistenceResult: """Outcome of running the persistence workflow.""" @@ -154,7 +155,7 @@ def render(self) -> str: return "; ".join(parts) -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class PersistenceFiles: """Backend and manifest file contents to persist.""" @@ -164,7 +165,7 @@ class PersistenceFiles: manifest_contents: dict[str, typ.Any] -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class PersistenceOptions: """Optional configuration and callbacks for persistence workflow.""" @@ -184,7 +185,7 @@ class PersistenceOptions: no_input: bool = False -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class PullRequestContext: """Context for opening a pull request.""" @@ -196,7 +197,7 @@ class PullRequestContext: pr_opener: typ.Callable[..., str | None] | None = None -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class PersistencePaths: """Resolved paths for manifest and backend files.""" @@ -204,7 +205,7 @@ class PersistencePaths: backend_path: Path -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class WorkspaceContext: """Working directory and repository for persistence operations.""" @@ -212,7 +213,7 @@ class WorkspaceContext: repository: pygit2.Repository -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class FinalizationContext: """Data needed to finalize persistence results and PR creation.""" diff --git a/concordat/persistence/validation.py b/concordat/persistence/validation.py index 82ef4a4..28b1cde 100644 --- a/concordat/persistence/validation.py +++ b/concordat/persistence/validation.py @@ -1,5 +1,4 @@ """Validation of user inputs and remote S3 backends.""" -# ruff: noqa: TRY003 from __future__ import annotations @@ -46,21 +45,29 @@ def _validate_inputs( def _validate_path_safety(path: str, field_name: str) -> None: """Ensure path segments do not include traversal elements.""" if ".." in path.split("/"): - raise PersistenceError(f"{field_name} may not include directory traversals.") + raise PersistenceError( # noqa: TRY003 # Domain error identifies invalid input. + f"{field_name} may not include directory traversals." + ) def _validate_key_suffix_not_empty(key_suffix: str) -> None: """Ensure the key suffix is not empty or whitespace only.""" if not key_suffix.strip(): - raise PersistenceError("Key suffix is required.") + raise PersistenceError( # noqa: TRY003 # Domain error identifies required input. + "Key suffix is required." + ) def _validate_required_fields(descriptor: PersistenceDescriptor) -> None: """Ensure required descriptor fields are populated.""" if not descriptor.bucket: - raise PersistenceError("Bucket is required.") + raise PersistenceError( # noqa: TRY003 # Domain error identifies required input. + "Bucket is required." + ) if not descriptor.region: - raise PersistenceError("Region is required.") + raise PersistenceError( # noqa: TRY003 # Domain error identifies required input. + "Region is required." + ) def _check_endpoint_scheme(endpoint: str, *, allow_insecure: bool) -> None: @@ -72,12 +79,12 @@ def _check_endpoint_scheme(endpoint: str, *, allow_insecure: bool) -> None: return if "://" not in endpoint: - raise PersistenceError( + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. "Endpoint must include an https:// scheme (for example, " "https://s3.example.com)." ) - raise PersistenceError( + raise PersistenceError( # noqa: TRY003 # Domain error provides operator remediation. "Endpoint must use HTTPS (for example, https://s3.example.com)." ) @@ -87,7 +94,9 @@ def _validate_endpoint_protocol( ) -> None: """Ensure endpoints use HTTPS unless explicitly allowed for dev use.""" if not (endpoint := endpoint.strip()): - raise PersistenceError("Endpoint is required.") + raise PersistenceError( # noqa: TRY003 # Domain error identifies required input. + "Endpoint is required." + ) endpoint = normalize_endpoint_url(endpoint) _check_endpoint_scheme(endpoint, allow_insecure=allow_insecure_endpoint) @@ -133,7 +142,7 @@ def _bucket_versioning_status(client: S3Client, bucket: str) -> str | None: f"Details: {error}" ) raise PersistenceError(message) from error - except boto_exceptions.ClientError as error: # type: ignore[attr-defined] + except boto_exceptions.ClientError as error: # type: ignore[attr-defined] # botocore defines ClientError dynamically. message = ( "Versioning check failed: the bucket API rejected the request. " "Confirm the bucket exists and the provided credentials can query it. " @@ -153,7 +162,7 @@ def _perform_s3_operation( except boto_exceptions.BotoCoreError as error: message = f"{error_message}: {error}" raise PersistenceError(message) from error - except boto_exceptions.ClientError as error: # type: ignore[attr-defined] + except boto_exceptions.ClientError as error: # type: ignore[attr-defined] # botocore defines ClientError dynamically. message = f"{error_message}: {error}" raise PersistenceError(message) from error @@ -177,12 +186,7 @@ def _session_token_from_environment(env: typ.Mapping[str, str]) -> str | None: def _credentials_from_environment(env: typ.Mapping[str, str]) -> dict[str, str]: - """Resolve S3 credentials from supported environment variables. - - Boto3 recognises the AWS_* variables. Concordat also supports alternative - names for S3-compatible vendors (Scaleway/Spaces) and maps those to the - boto3 client arguments. - """ + """Resolve S3 credentials from AWS and supported vendor environment variables.""" def present(*names: str) -> bool: return all(env.get(name, "").strip() for name in names) diff --git a/concordat/platform_standards.py b/concordat/platform_standards.py index 931b716..145b34c 100644 --- a/concordat/platform_standards.py +++ b/concordat/platform_standards.py @@ -48,12 +48,12 @@ def pull_requests( _yaml.sort_base_mapping_type_on_output = False ERROR_MISSING_TOKEN = ( - "GITHUB_TOKEN is required to open the platform-standards pull request" # noqa: S105 + "GITHUB_TOKEN is required to open the platform-standards pull request" # noqa: S105 # Error text only; no secret value. ) ERROR_SLUG = "Unable to determine GitHub slug from URL" -@dataclasses.dataclass +@dataclasses.dataclass(slots=True) class PlatformStandardsConfig: """Configuration for interacting with the platform-standards repository.""" @@ -63,7 +63,7 @@ class PlatformStandardsConfig: github_token: str | None = None -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class PlatformStandardsResult: """Outcome of attempting to open a platform-standards pull request.""" @@ -90,7 +90,9 @@ def _check_base_branch_enrollment( branch_name: Branch name for the result. expect_present: If True, return early when present; if False, when absent. - Returns: + Returns + ------- + PlatformStandardsResult | None PlatformStandardsResult if already in expected state, None otherwise. """ @@ -134,7 +136,9 @@ def _create_pr_for_inventory_change( branch_name: Branch name for the PR. verb: Action verb ("enrol" or "disenrol"). - Returns: + Returns + ------- + tuple[str, str] Tuple of (pr_url, message). """ @@ -180,7 +184,9 @@ def _handle_existing_remote_branch( expect_present: If True (enrol), slug present; if False (disenrol), absent. verb: Action verb ("enrol" or "disenrol"). - Returns: + Returns + ------- + PlatformStandardsResult | None PlatformStandardsResult if branch exists with change, None otherwise. """ @@ -267,29 +273,53 @@ def _validate_tofu_changes(workdir: Path) -> None: _run_tofu_validate(workdir) -def _ensure_inventory_pr( +def _apply_inventory_change( + repository: pygit2.Repository, + workdir: Path, + config: PlatformStandardsConfig, repo_slug: str, + base_commit: pygit2.Commit, *, - config: PlatformStandardsConfig, verb: str, mutate_inventory: typ.Callable[[Path, str], bool], - expect_present: bool, - already_message: str, -) -> PlatformStandardsResult: - """Shared implementation for inventory PR creation (enrol/disenrol). +) -> bool: + """Apply an inventory mutation, commit it, and validate the changes. Args: - repo_slug: Repository slug to add/remove from inventory. + repository: The pygit2 repository object. + workdir: Working directory containing the inventory. config: Platform standards configuration. + repo_slug: Repository slug being changed. + base_commit: The base commit to create the new commit on. verb: Action verb ("enrol" or "disenrol"). mutate_inventory: Function to modify inventory (returns True if changed). - expect_present: Whether slug should be present on base branch for early-out. - already_message: Message when inventory mutation returns no change. - Returns: - PlatformStandardsResult with PR details or early-out explanation. + Returns + ------- + bool + True when the inventory changed and was committed and validated, otherwise + False. """ + inventory_path = workdir / config.inventory_path + if not mutate_inventory(inventory_path, repo_slug): + return False + + _commit_inventory_changes(repository, config, repo_slug, base_commit, verb=verb) + _validate_tofu_changes(workdir) + return True + + +def _ensure_inventory_pr( + repo_slug: str, + *, + config: PlatformStandardsConfig, + verb: str, + mutate_inventory: typ.Callable[[Path, str], bool], + expect_present: bool, + already_message: str, +) -> PlatformStandardsResult: + """Create an inventory PR for an enrolment or removal change.""" callbacks = build_remote_callbacks(config.repo_url) with TemporaryDirectory(prefix="concordat-platform-") as temp_root: repository = pygit2.clone_repository( @@ -297,7 +327,6 @@ def _ensure_inventory_pr( temp_root, callbacks=callbacks, ) - workdir = Path(repository.workdir or temp_root) remote = repository.remotes["origin"] remote.fetch(callbacks=callbacks) @@ -326,9 +355,15 @@ def _ensure_inventory_pr( branch_name=branch_name, ) - inventory_path = workdir / config.inventory_path - changed = mutate_inventory(inventory_path, repo_slug) - if not changed: + if not _apply_inventory_change( + repository, + workdir, + config, + repo_slug, + base_commit, + verb=verb, + mutate_inventory=mutate_inventory, + ): return PlatformStandardsResult( created=False, branch=branch_name, @@ -336,9 +371,6 @@ def _ensure_inventory_pr( message=already_message, ) - _commit_inventory_changes(repository, config, repo_slug, base_commit, verb=verb) - _validate_tofu_changes(workdir) - if not config.github_token: raise ConcordatError(ERROR_MISSING_TOKEN) @@ -395,6 +427,11 @@ def _load_inventory_data(path: Path) -> dict[str, typ.Any]: Always returns a dict with at least schema_version and repositories keys, even if the file doesn't exist or contains invalid data. + Returns + ------- + dict[str, typ.Any] + Normalized inventory mapping. + """ if not path.exists(): return {"schema_version": 1, "repositories": []} @@ -414,6 +451,11 @@ def _build_canonical_inventory( Preserves any extra keys from the original data that aren't in the base structure. + Returns + ------- + dict[str, typ.Any] + Canonical inventory mapping with stable key ordering. + """ schema_version = int(data.get("schema_version", 1) or 1) canonical: dict[str, typ.Any] = { @@ -449,7 +491,9 @@ def _update_inventory(path: Path, repo_slug: str) -> bool: def _load_and_validate_inventory_data(path: Path) -> dict[str, typ.Any] | None: """Load and validate inventory file structure, returning data or None. - Returns: + Returns + ------- + dict[str, typ.Any] | None Dictionary with inventory data if valid, None if file missing or invalid. """ @@ -473,7 +517,9 @@ def _filter_repository_entries( repos_raw: Raw repositories value from YAML (may be any type). repo_slug: Repository slug to remove. - Returns: + Returns + ------- + tuple[list[dict[str, typ.Any]], bool] Tuple of (filtered_list, changed) where changed is True if removed. """ @@ -515,7 +561,7 @@ def _remove_inventory(path: Path, repo_slug: str) -> bool: def _run_cmd(args: list[str], *, cwd: Path) -> None: - subprocess.run( # noqa: S603 + subprocess.run( # noqa: S603 # Fixed argv, no shell. args, check=True, cwd=str(cwd), @@ -570,8 +616,15 @@ def _resolve_branch_commit( ) -> pygit2.Commit: """Return the commit for either a local or remote branch reference. - Raises: - KeyError: If branch exists neither locally nor as origin/{branch_name}. + Returns + ------- + pygit2.Commit + Commit resolved from the local or remote branch reference. + + Raises + ------ + KeyError + If branch exists neither locally nor as origin/{branch_name}. """ try: @@ -640,6 +693,11 @@ def _checkout_pr_branch( When a prior concordat run already pushed the branch, recreating it from the base branch can lead to non-fast-forward push failures. Reusing the remote branch keeps the push linear and updates any existing PR. + + Returns + ------- + pygit2.Commit + Commit checked out as the base of the work branch. """ remote = repository.remotes["origin"] remote.fetch(callbacks=callbacks) @@ -674,9 +732,15 @@ def _open_or_fetch_pull_request( ) -> _PullRequest: """Create a pull request, or return the existing one for the same head. - Raises: - github3.exceptions.UnprocessableEntity: If PR creation fails and no - existing PR is found for the same head branch. + Returns + ------- + _PullRequest + Existing or newly created pull request. + + Raises + ------ + ImportError + If the installed github3 package lacks the expected exception type. """ github3_exceptions = getattr(github3, "exceptions", None) diff --git a/concordat/rules/envelope.py b/concordat/rules/envelope.py index 65fbfda..fb73ff9 100644 --- a/concordat/rules/envelope.py +++ b/concordat/rules/envelope.py @@ -17,7 +17,7 @@ # The parsed Cargo manifest is opaque to the policy (only its presence matters), # so it is modelled as an arbitrary TOML table rather than a fixed schema. -CargoManifest = dict[str, object] +type CargoManifest = dict[str, object] class CargoPayload(typ.TypedDict): @@ -78,6 +78,12 @@ def build_envelope(checkout: pathlib.Path) -> PolicyEnvelope: Root `Cargo.toml` presence is provisional evidence of Rust applicability; the `.concordat` manifest remains the eventual authority (see the Parabellum ExecPlan decision log). + + Returns + ------- + PolicyEnvelope + Policy input document assembled from the checkout. + """ cargo_path = checkout / "Cargo.toml" makefile_path = checkout / "Makefile" diff --git a/concordat/rules/makefile_facts.py b/concordat/rules/makefile_facts.py index b0f7b0a..cdd8f9b 100644 --- a/concordat/rules/makefile_facts.py +++ b/concordat/rules/makefile_facts.py @@ -139,6 +139,16 @@ def _run_makeutil( The subprocess runs with the Makefile's directory as its working directory so the recorded ``source.path`` stays repository-relative. + + Returns + ------- + subprocess.CompletedProcess[str] + Completed makeutil process result. + + Raises + ------ + _makeutil_error + If makeutil cannot be started. """ try: return subprocess.run( # noqa: S603 - fixed argv, no shell @@ -260,6 +270,11 @@ def _require_bool(value: object, label: str, path: pathlib.Path) -> None: Checked before `isinstance(value, int)` would matter: `bool` is a subclass of `int`, so an explicit type check is what keeps `1` from passing as `True`. + + Raises + ------ + _malformed + If *value* is not a boolean. """ if not isinstance(value, bool): raise _malformed(label, path) @@ -277,6 +292,11 @@ def _require_location(value: object, label: str, path: pathlib.Path) -> None: The policy uses ``start_line`` as a finding's reported line. A bool would satisfy `isinstance(..., int)`, so it is excluded explicitly. + + Raises + ------ + _malformed + If *value* lacks a valid integer ``start_line``. """ location = _require_object(value, label, path) start_line = location.get("start_line") @@ -350,6 +370,12 @@ def inspect_makefile(path: pathlib.Path, *, timeout: float = 10.0) -> MakefileFa ``variables``/``includes`` shapes — is validated before exit-code/status agreement. The decoded mapping is only narrowed to :class:`MakeutilReport` once every required shape has been checked. + + Returns + ------- + MakefileFacts + Validated Makefile facts from the makeutil report. + """ completed = _run_makeutil(path, timeout) _validate_exit_code(completed, path) diff --git a/concordat/rules/runner.py b/concordat/rules/runner.py index 770b8cc..a3b004b 100644 --- a/concordat/rules/runner.py +++ b/concordat/rules/runner.py @@ -28,6 +28,11 @@ def _resolve_rule_packages_dir() -> pathlib.Path: ``pyproject.toml``), reachable via ``importlib.resources``. A source checkout keeps them in the sibling ``platform-standards`` tree, so that layout is used as a fallback. + + Returns + ------- + pathlib.Path + Directory containing the canon lint-rule packages. """ packaged = importlib.resources.files("concordat") / "canon" / "lint-rules" if isinstance(packaged, pathlib.Path) and packaged.is_dir(): @@ -138,7 +143,13 @@ class RuleRunResult: @property def exit_code(self) -> int: - """Return 0 when compliant; 1 when any finding exists (fail closed).""" + """Process exit code: 0 when compliant, otherwise 1 (fail closed). + + Returns + ------- + int + ``0`` when compliant; otherwise ``1``. + """ return 0 if self.verdict == VERDICT_COMPLIANT else 1 @@ -174,6 +185,16 @@ def _rule_package_dir(rule_id: str) -> pathlib.Path: The packages root is resolved here rather than at import, so a missing or unreadable rule tree fails when a rule is run rather than when the module is imported — importing the CLI should not depend on the policy tree. + + Returns + ------- + pathlib.Path + Directory containing the requested rule package. + + Raises + ------ + OperationalRuleError + If *rule_id* is invalid or its package directory is unavailable. """ # Validation first: a malformed identifier is a local error, and must not # cost the packages-root lookup (which touches the filesystem) to reject. @@ -210,6 +231,16 @@ def _rule_parameters(rule_dir: pathlib.Path) -> dict[str, typ.Any]: The policies read their tunables from ``data.parameters``; without this the manifest's declared defaults would be inert and only the ``default`` rules baked into the Rego would ever apply. + + Returns + ------- + dict[str, typ.Any] + Parameter defaults declared by the rule manifest. + + Raises + ------ + OperationalRuleError + If the rule manifest cannot be read or is malformed. """ manifest_path = rule_dir / "rule.yaml" if not manifest_path.is_file(): @@ -287,6 +318,12 @@ def _require_policy_exit_code( A higher status means Conftest could not evaluate it — a malformed policy, a bad flag, a missing file — and it may still print well-formed JSON on stdout. Decoding that would report an operational failure as a clean run. + + Raises + ------ + OperationalRuleError + If Conftest exits without producing a policy verdict. + """ if completed.returncode in POLICY_EXIT_CODES: return @@ -545,14 +582,12 @@ def render_table(result: RuleRunResult) -> str: widths = [max(len(row[column]) for row in rows) for column in range(3)] lines = [header] lines.extend( - " ".join( - ( - row[0].ljust(widths[0]), - row[1].ljust(widths[1]), - row[2].ljust(widths[2]), - row[3], - ) - ) + " ".join(( + row[0].ljust(widths[0]), + row[1].ljust(widths[1]), + row[2].ljust(widths[2]), + row[3], + )) for row in rows ) return "\n".join(lines) diff --git a/concordat/runtime.py b/concordat/runtime.py new file mode 100644 index 0000000..9a72fa8 --- /dev/null +++ b/concordat/runtime.py @@ -0,0 +1,19 @@ +"""Bind the public greeting to its native implementation when available. + +``concordat.hello`` re-exports this module's ``hello`` binding. Concordat uses +``_concordat_rs.hello`` when the optional Rust extension is installed and falls +back to ``.pure.hello`` when the extension itself is absent. Import failures +from the extension's dependencies propagate to callers unchanged. +""" + +import importlib +import typing as typ + +type Hello = typ.Callable[[], str] + +try: # pragma: no cover - Rust optional + hello = typ.cast("Hello", importlib.import_module("_concordat_rs").hello) +except ModuleNotFoundError as exc: # pragma: no cover - Python fallback + if exc.name != "_concordat_rs": + raise + from .pure import hello as hello diff --git a/concordat/tofu_output.py b/concordat/tofu_output.py index 2ade713..4695734 100644 --- a/concordat/tofu_output.py +++ b/concordat/tofu_output.py @@ -20,7 +20,9 @@ def normalize_init_result(result: object) -> SimpleNamespace: Args: result: The result from tofupy.init(), typically a boolean. - Returns: + Returns + ------- + SimpleNamespace SimpleNamespace with stdout, stderr, and returncode. """ @@ -33,7 +35,9 @@ def normalize_plan_result(result: object) -> SimpleNamespace: Args: result: The result from tofupy.plan(), a (PlanLog, Plan) tuple. - Returns: + Returns + ------- + SimpleNamespace SimpleNamespace with stdout, stderr, and returncode. """ @@ -65,7 +69,9 @@ def normalize_apply_result(result: object) -> SimpleNamespace: Args: result: The result from tofupy.apply(), an ApplyLog. - Returns: + Returns + ------- + SimpleNamespace SimpleNamespace with stdout, stderr, and returncode. """ @@ -101,7 +107,9 @@ def _summarize_tofu_log( verb: The tofu command verb (plan, apply, etc.). log: The structured log object from tofupy. - Returns: + Returns + ------- + tuple[str, str, bool] A tuple of (stdout, stderr, errored). """ @@ -138,7 +146,9 @@ def _format_tofu_diagnostics(errors: list[object], warnings: list[object]) -> st errors: List of error diagnostic objects. warnings: List of warning diagnostic objects. - Returns: + Returns + ------- + str Formatted string for terminal output. """ @@ -169,7 +179,9 @@ def normalize_tofu_result(verb: str, result: object) -> SimpleNamespace: verb: The tofu command verb (init, plan, apply, etc.). result: The result from tofupy command execution. - Returns: + Returns + ------- + SimpleNamespace SimpleNamespace with stdout, stderr, and returncode attributes. """ diff --git a/concordat/tofu_runner.py b/concordat/tofu_runner.py index 60b95ea..b84141d 100644 --- a/concordat/tofu_runner.py +++ b/concordat/tofu_runner.py @@ -36,6 +36,12 @@ def resolve_tofu_workdir(workspace_root: Path) -> Path: the bundled `platform-standards` template). Some tests and legacy layouts place configuration at the repository root, so we fall back to the root when `tofu/` is absent or does not appear to contain OpenTofu files. + + Returns + ------- + Path + Directory containing the OpenTofu root module. + """ candidate = workspace_root / TOFU_DIRNAME if not candidate.is_dir(): diff --git a/concordat/tofu_yaml.py b/concordat/tofu_yaml.py index 192d231..03f45c0 100644 --- a/concordat/tofu_yaml.py +++ b/concordat/tofu_yaml.py @@ -31,7 +31,9 @@ def strip_yaml_directives_for_tofu(contents: str) -> tuple[str, bool]: This function performs a minimal, surgical rewrite that only strips markers at the beginning/end of the file so we do not rewrite the entire document. - Returns: + Returns + ------- + tuple[str, bool] A tuple of (sanitized_contents, changed) where changed is True if any modifications were made. @@ -89,7 +91,9 @@ def sanitize_yaml_file_for_tofu(path: Path) -> bool: Args: path: Path to a YAML file to sanitize in-place. - Returns: + Returns + ------- + bool True if the file was modified, False otherwise. """ @@ -115,6 +119,12 @@ def _build_inventory_candidates( Returns paths in both workspace root and tofu working directory, adjusting the inventory path if it starts with the tofu dirname. + + Returns + ------- + list[Path] + Candidate inventory paths to inspect. + """ candidates: list[Path] = [workspace_root / inventory_path] if tofu_workdir.resolve() != workspace_root.resolve(): @@ -129,6 +139,12 @@ def _sanitize_candidates(candidates: list[Path]) -> bool: """Sanitize all existing inventory files in the candidates list. Returns True if any file was modified, False otherwise. + + Returns + ------- + bool + Whether any candidate file was modified. + """ changed = False for candidate in candidates: @@ -152,7 +168,9 @@ def sanitize_inventory_for_tofu( tofu_workdir: Directory containing tofu configuration. inventory_path: Relative path to the inventory file. - Returns: + Returns + ------- + bool True if any inventory file was modified, False otherwise. """ diff --git a/concordat/user_interaction.py b/concordat/user_interaction.py index aeb1657..c6dde41 100644 --- a/concordat/user_interaction.py +++ b/concordat/user_interaction.py @@ -20,7 +20,9 @@ def prompt_yes_no(message: str, output: typ.IO[str] | None = None) -> bool: message: The prompt message to display. output: Output stream for the prompt (defaults to stderr). - Returns: + Returns + ------- + bool True if the user responded with 'y' or 'yes', False otherwise. """ @@ -37,7 +39,9 @@ def prompt_yes_no(message: str, output: typ.IO[str] | None = None) -> bool: def can_prompt() -> bool: """Check if interactive prompting is available. - Returns: + Returns + ------- + bool True if stdin is connected to a TTY, False otherwise. """ diff --git a/docs/concordat-design.md b/docs/concordat-design.md index 067d453..b2ce84b 100644 --- a/docs/concordat-design.md +++ b/docs/concordat-design.md @@ -220,8 +220,8 @@ false-positive rate is acceptable. Exemptions use the existing state changes to IaC. See [`docs/developers-guide.md`](developers-guide.md) for the CLI's internal -module boundaries — XDG layout, credential resolution, the cache/execution -API split, and the rule-run and Parabellum sweep contracts. +module boundaries — XDG layout, credential resolution, the cache/execution API +split, and the rule-run and Parabellum sweep contracts. ### 2.7 Estate execution workflow @@ -447,10 +447,10 @@ provider reuses the AWS env var contract. The design deliberately omits `encrypt = true` because Terraform's backend sends an AES256 (SSE-S3) header with that flag, and neither Scaleway nor DigitalOcean Spaces accepts it. Scaleway also offers SSE-ONE and SSE-KMS in addition to SSE-C, but bucket -encryption for Scaleway is configured separately rather than through -Terraform's `encrypt` flag. At-rest encryption therefore remains a caller -concern (for example, by configuring bucket-side encryption directly, -keeping secrets out of state, or using client-side encryption). +encryption for Scaleway is configured separately rather than through Terraform's +`encrypt` flag. At-rest encryption therefore remains a caller concern (for +example, by configuring bucket-side encryption directly, keeping secrets out of +state, or using client-side encryption). Every persistence descriptor ships alongside a YAML manifest (`platform-standards/tofu/backend/persistence.yaml`) storing a schema version, @@ -1134,17 +1134,16 @@ Commands must return stable exit codes for CI integration. | 4 | Mutations planned or applied failed (patch application or policy errors) | Table 3 is a proposal for the general `artefact` command family and does not -describe what ships today. The one `artefact` subcommand that is -implemented, `concordat artefact rule run`, ships a narrower, already-fixed -scheme instead: +describe what ships today. The one `artefact` subcommand that is implemented, +`concordat artefact rule run`, ships a narrower, already-fixed scheme instead: - `0` — compliant. - `1` — policy findings, including `indeterminate` (which fails closed). - `2` — operational failure. -See [`docs/developers-guide.md`, "Verdicts and exit -codes"](developers-guide.md#verdicts-and-exit-codes) for the mapping from -verdict to exit code. +See +[`docs/developers-guide.md`, "Verdicts and exit codes"](developers-guide.md#verdicts-and-exit-codes) +for the mapping from verdict to exit code. ##### Configuration and locking @@ -1361,8 +1360,8 @@ declared, the invocation must be qualified to a surface — either a `cd &&` prefix or a `--manifest-path ` flag — and QG-001 requires every declared surface's gate to be reachable from `lint`. -`make -C ` is indeterminate in every role, both as a reachability edge -and as a surface qualifier. The two qualifiers above keep the gate invocation +`make -C ` is indeterminate in every role, both as a reachability edge and +as a surface qualifier. The two qualifiers above keep the gate invocation inside the file being parsed, so it remains a fact; `-C` instead delegates to a Makefile this rule never reads, and accepting it would assert a gate that has not been observed. Treating it as proof would need the envelope to carry the diff --git a/docs/cyclopts-users-guide.md b/docs/cyclopts-users-guide.md index 08e629a..816665d 100644 --- a/docs/cyclopts-users-guide.md +++ b/docs/cyclopts-users-guide.md @@ -49,10 +49,12 @@ ______________________________________________________________________ ```python import cyclopts + def greet(name: str, count: int = 1): for _ in range(count): print(f"Hello, {name}!") + if __name__ == "__main__": cyclopts.run(greet) ``` @@ -68,18 +70,22 @@ from cyclopts import App app = App(help="Demo multi‑command app") + @app.command def fizz(n: int): print(f"FIZZ: {n}") + @app.command(alias="buzz") def buzz_renamed(n: int): print(f"BUZZ: {n}") + @app.default def main(): print("Use a subcommand; try --help") + if __name__ == "__main__": app() ``` @@ -101,13 +107,13 @@ from cyclopts import App, Parameter app = App() + @app.command def build( *, profile: Annotated[str, Parameter(name=["--profile", "-p"])], out_dir: Annotated[str, Parameter(name="--out-dir", alias=["-o"])], -): - ... +): ... ``` - **Docstrings** should document the *Python variable names*, even if CLI names @@ -129,6 +135,7 @@ from cyclopts import App, Parameter app = App() + @app.default def main(verbose: Annotated[int, Parameter(alias="-v", count=True)] = 0): print(f"verbosity={verbose}") @@ -174,6 +181,7 @@ from cyclopts import App, Parameter, Token, validators app = App() UNITS = {"kb": 1024, "mb": 1024**2, "gb": 1024**3} + def bytesize(_type, tokens: Sequence[Token]) -> int: s = tokens[0].value.lower() try: @@ -182,9 +190,13 @@ def bytesize(_type, tokens: Sequence[Token]) -> int: number, suffix = s[:-2], s[-2:] return int(number) * UNITS[suffix] + @app.command -def zero(size: Annotated[int, Parameter(converter=bytesize)], *, - at_least: Annotated[int, Parameter(validator=validators.Number(gte=0))] = 0): +def zero( + size: Annotated[int, Parameter(converter=bytesize)], + *, + at_least: Annotated[int, Parameter(validator=validators.Number(gte=0))] = 0, +): assert size >= at_least, "size below minimum" ``` @@ -208,12 +220,14 @@ from cyclopts import App, Parameter app = App() + @dataclass class User: name: str age: int region: Literal["us", "ca"] = "us" + @app.default def show(user: Annotated[User, Parameter(name="*")]): print(user) @@ -234,15 +248,19 @@ from typing import Annotated from cyclopts import App, Group, Parameter, validators app = App() -vehicle = Group("Vehicle (choose one)", - validator=validators.LimitedChoice(), - default_parameter=Parameter(negative="")) +vehicle = Group( + "Vehicle (choose one)", + validator=validators.LimitedChoice(), + default_parameter=Parameter(negative=""), +) + @app.command -def create(*, - car: Annotated[bool, Parameter(group=vehicle)] = False, - truck: Annotated[bool, Parameter(group=vehicle)] = False): - ... +def create( + *, + car: Annotated[bool, Parameter(group=vehicle)] = False, + truck: Annotated[bool, Parameter(group=vehicle)] = False, +): ... ``` Set `Group.sort_key` or use `Group.create_ordered()` to control panel ordering @@ -268,6 +286,7 @@ For packaged apps: ```python from cyclopts import App + app = App(name="myapp") app.register_install_completion_command() # adds --install-completion if __name__ == "__main__": @@ -300,9 +319,11 @@ from cyclopts import App, config app = App( name="character-counter", config=[ - config.Toml("pyproject.toml", - root_keys=["tool", "character-counter"], - search_parents=True), + config.Toml( + "pyproject.toml", + root_keys=["tool", "character-counter"], + search_parents=True, + ), config.Env("CHAR_COUNTER_"), ], ) @@ -342,10 +363,15 @@ ______________________________________________________________________ ## 11) Calling apps, exits, and return values ```python -app = App(result_action="return_value") # don’t sys.exit; return value instead +app = App(result_action="return_value") # don’t sys.exit; return value instead + + @app.command -def add(a: int, b: int) -> int: return a + b -rv = app(["add", "2", "3"]) # 5 +def add(a: int, b: int) -> int: + return a + b + + +rv = app(["add", "2", "3"]) # 5 ``` When `result_action` is not set, Cyclopts mirrors installed entry‑point @@ -371,16 +397,20 @@ from cyclopts import App, Parameter app = App() + @app.command def whoami(user: str): print(user) + @app.meta.default -def launcher(*tokens: Annotated[str, Parameter(show=False, allow_leading_hyphen=True)], - user: str): +def launcher( + *tokens: Annotated[str, Parameter(show=False, allow_leading_hyphen=True)], user: str +): # do auth / logging / inject defaults here app(tokens) # forward to the real app + if __name__ == "__main__": app.meta() ``` @@ -413,10 +443,12 @@ from cyclopts import App app = App(result_action="return_value") + @app.command def add(a: int, b: int) -> int: return a + b + def test_add(): assert app(["add", "2", "3"]) == 5 ``` @@ -449,25 +481,32 @@ app = App( ) paths = Group.create_ordered("Paths") -opts = Group.create_ordered("Options") +opts = Group.create_ordered("Options") + @dataclass class Resize: width: int height: int + @Parameter(name="*") @dataclass class Global: verbose: bool = False profile: Literal["debug", "release"] = "debug" + @app.command(group="Transforms") -def resize(src: Annotated[Path, Parameter(group=paths, validator=validators.Path(exists=True))], - dst: Annotated[Path, Parameter(group=paths)], - *, - size: Annotated[Resize, Parameter(name="size")], - global_: Annotated[Global, Parameter(name="*")]): +def resize( + src: Annotated[ + Path, Parameter(group=paths, validator=validators.Path(exists=True)) + ], + dst: Annotated[Path, Parameter(group=paths)], + *, + size: Annotated[Resize, Parameter(name="size")], + global_: Annotated[Global, Parameter(name="*")], +): """Resize an image. Parameters @@ -484,9 +523,12 @@ def resize(src: Annotated[Path, Parameter(group=paths, validator=validators.Path Verbose logging. """ if global_.verbose: - print(f"Resizing {src} -> {dst} to {size.width}×{size.height} [{global_.profile}]") + print( + f"Resizing {src} -> {dst} to {size.width}×{size.height} [{global_.profile}]" + ) # process... + if __name__ == "__main__": app() ``` diff --git a/docs/developers-guide.md b/docs/developers-guide.md index c67381c..ff51648 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1,40 +1,86 @@ # Concordat developers' guide -This guide documents concordat's internal boundaries: the module contracts -that other modules, tests, and the CLI rely on. It complements -[`docs/users-guide.md`](users-guide.md), which describes CLI behaviour from -an operator's perspective, and -[`docs/concordat-design.md`](concordat-design.md), which specifies the -broader estate-audit architecture. Where behaviour described here is planned -rather than shipped, that is called out explicitly; everything else is -derived from the current source. +This guide documents concordat's internal boundaries: the module contracts that +other modules, tests, and the CLI rely on. It complements +[`docs/users-guide.md`](users-guide.md), which describes CLI behaviour from an +operator's perspective, and [`docs/concordat-design.md`](concordat-design.md), +which specifies the broader estate-audit architecture. Where behaviour +described here is planned rather than shipped, that is called out explicitly; +everything else is derived from the current source. ## Development environment and gates -`uv sync --group dev` installs the development dependency group. The group -is declared in `pyproject.toml` under `[dependency-groups]` as `dev`, and -pulls in pytest, pytest-xdist, pytest-bdd, pytest-asyncio, pytest-mock, -ruff, pyright, pytest-timeout, betamax, hypothesis, and textual. The -`Makefile`'s `build` target runs `uv sync --group dev` as part of setting up -the virtual environment. +`uv sync --group dev` installs the development dependency group. The group is +declared in `pyproject.toml` under `[dependency-groups]` as `dev`, and pulls in +pytest, pytest-xdist, pytest-bdd, pytest-asyncio, pytest-mock, ruff, pyright, +pytest-timeout, betamax, hypothesis, textual, and the pinned +`df12-python-lints` plugin at immutable commit +`9c835f35b0f1690597ade799c9c6a30bc5922959` (lock metadata version 0.1.0). +The `Makefile`'s `build` target runs +`uv sync --group dev` as part of setting up the virtual environment. + +`make lint` runs four complementary checks. Ruff provides the fast source-wide +style and correctness pass, including preview, asynchronous, and +NumPy-docstring rules. Pylint then runs the selected Lading policy through the +pinned PyPy shim. A separate CPython 3.14 invocation loads every diagnostic +from the `df12-python-lints` pin, while retaining Concordat's Python 3.13 +semantic baseline for version-gated checks. Finally, `ambrleaks`, provisioned +from the same immutable release, scans the test tree for unredacted values in +Syrupy snapshots. The separate df12 process prevents its CPython dependency +from changing the PyPy-backed Pylint baseline. + +## Public runtime boundary + +`concordat.hello` is the public greeting entry point. At runtime it selects +`_concordat_rs.hello` when the optional Rust extension is installed and falls +back to `.pure.hello` only when importing `_concordat_rs` itself raises +`ModuleNotFoundError`. A missing dependency reported while importing the native +extension is re-raised, so packaging and environment failures remain visible. +`Hello` in `concordat.runtime` is an internal typing alias, not part of the +public API. + +## Platform-standards inventory mutation boundary + +`_apply_inventory_change` defines the sequencing contract for inventory pull +request changes: + +1. Call the supplied mutation with the configured inventory path and repository + slug. +2. If it reports no change, return without creating a commit or running + validation. +3. Commit the changed inventory. +4. Run the validation boundary (`tofu fmt`, its check mode, `tflint`, and + `tofu validate`). + +The helper returns `True` only after a changed inventory has been committed and +validated. + +## Canonical artefact TUI refresh boundary + +The canonical-artefact TUI's `action_refresh` method recomputes comparisons +from the manifest and published checkout using the existing filters, updates +the comparison state, and clears and repopulates the mounted `DataTable` in +place. Refresh therefore reuses the existing application and table rather than +replacing either widget; the refresh and sync key bindings depend on that +mounted table. The type checker is pinned, not resolved at run time. `Makefile` declares `TY_VERSION ?= 0.0.65` and `TY := uv tool run ty@$(TY_VERSION)`; the -`typecheck` target invokes `$(TY)` throughout. An unpinned `ty` meant CI and -a local checkout could run different versions of the tool and disagree -about which diagnostics were real; pinning the version in one Makefile -variable, and having every invocation read it from there, closes that gap. -`ty` is deliberately absent from the Makefile's `TOOLS` list — the CLI -tools whose presence `make` verifies with `command -v` — because it is -fetched on demand at the pinned version via `uv tool run` instead of being -expected to already be on `PATH`. +`typecheck` target invokes `$(TY)` throughout. An unpinned `ty` meant CI and a +local checkout could run different versions of the tool and disagree about +which diagnostics were real; pinning the version in one Makefile variable, and +having every invocation read it from there, closes that gap. `ty` is +deliberately absent from the Makefile's `TOOLS` list — the CLI tools whose +presence `make` verifies with `command -v` — because it is fetched on demand at +the pinned version via `uv tool run` instead of being expected to already be on +`PATH`. ## XDG layout and owner namespaces -`concordat/xdg.py` is the single source of truth for where concordat reads -and writes. Three roots are resolved from the XDG base-directory environment -variables, each falling back to the conventional default when the variable -is unset, relative, or empty (the XDG specification requires relative base +`concordat/xdg.py` is the single source of truth for where concordat reads and +writes. Three roots are resolved from the XDG base-directory environment +variables, each falling back to the conventional default when the variable is +unset, relative, or empty (the XDG specification requires relative base directories to be ignored): - `config_root()` — `$XDG_CONFIG_HOME/concordat`, falling back to @@ -56,26 +102,26 @@ one of these roots: - `owner_state_dir` / `owner_runs_dir` — `owners//runs` under the state root, holding throwaway OpenTofu working trees. -The OpenTofu provider plugin cache -(`tofu_plugin_cache_dir`, `$XDG_CACHE_HOME/concordat/tofu/plugin-cache`) is -the one cache path that is *not* owner-namespaced: provider binaries are the -same regardless of which owner's estate is being planned. +The OpenTofu provider plugin cache (`tofu_plugin_cache_dir`, +`$XDG_CACHE_HOME/concordat/tofu/plugin-cache`) is the one cache path that is +*not* owner-namespaced: provider binaries are the same regardless of which +owner's estate is being planned. -The **active owner** — the owner selected by `concordat owner use ` — -is not itself namespaced. It is the single `github_owner` key in the -**headline configuration file**, `$XDG_CONFIG_HOME/concordat/config.yaml` +The **active owner** — the owner selected by `concordat owner use ` — is +not itself namespaced. It is the single `github_owner` key in the **headline +configuration file**, `$XDG_CONFIG_HOME/concordat/config.yaml` (`headline_config_path`). `get_active_owner` reads that key; `set_active_owner` validates the owner and rewrites the file, preserving any other keys already present so the headline file can grow additional settings without one writer clobbering another's. -Every owner-derived path is built through `validate_owner`, which accepts -names that begin and end with an alphanumeric character and may contain -alphanumerics and hyphens internally, including doubled internal hyphens -(the pattern is `^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?$`). Owner names -reach this validation before they are joined into a filesystem path, so a -malformed owner argument fails fast rather than producing a path that -quietly bypasses namespacing. +Every owner-derived path is built through `validate_owner`, which accepts names +that begin and end with an alphanumeric character and may contain alphanumerics +and hyphens internally, including doubled internal hyphens (the pattern is +`^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?$`). Owner names reach this +validation before they are joined into a filesystem path, so a malformed owner +argument fails fast rather than producing a path that quietly bypasses +namespacing. ### Legacy-flat migration @@ -85,18 +131,18 @@ same file that is now the headline config: headline config are the same `config.yaml`** — migration does not move data between files so much as separate two concerns that used to share one file. -`concordat.estate_config.migrate_legacy_config` is the explicit, -side-effecting migration step, invoked once at CLI bootstrap -(`concordat.cli.main`) so that `default_config_path` stays a pure read-only -query for every command. It is a no-op once an active owner is already -configured, once the flat file has no `estate` section, or once the estate -section cannot be attributed to exactly one owner: +`concordat.estate_config.migrate_legacy_config` is the explicit, side-effecting +migration step, invoked once at CLI bootstrap (`concordat.cli.main`) so that +`default_config_path` stays a pure read-only query for every command. It is a +no-op once an active owner is already configured or once the flat file has no +`estate` section. If the estate section cannot be attributed to exactly one +owner, migration raises an error: - `_derive_owner_from_estates` collects every `github_owner` recorded across the legacy estates. The legacy format permitted estates for more than one - owner in one file; migrating such a section under the first owner - encountered would silently misplace the other owners' estates, so - mixed-owner input is rejected with an error rather than migrated. + owner in one file; migrating such a section under the first owner encountered + would silently misplace the other owners' estates, so mixed-owner input is + rejected with an error rather than migrated. When migration proceeds, the steps run in this deliberate order: @@ -105,33 +151,35 @@ When migration proceeds, the steps run in this deliberate order: 2. **Set the active owner.** The headline file's `github_owner` key is set to the derived owner. 3. **Remove the legacy estate section last.** The `estate` key is dropped - from the flat file (rewriting it if other keys remain, deleting it - outright if the estate section was its only content). + from the flat file (rewriting it if other keys remain, deleting it outright + if the estate section was its only content). This order is load-bearing, and the ordering is deliberate rather than incidental: - The active owner is what points `default_config_path` at the newly - migrated, owner-scoped file. Setting it only *after* the owner-scoped - write is complete means a reader never observes an active owner whose - file is not yet populated. -- Cleanup is the only step allowed to fail, because it is placed last. Were - the legacy section removed first, a failure between that removal and the - owner-scoped write would leave the estates in neither place the CLI - looks: the flat file no longer holds them, and no active owner yet - selects the owner-scoped file — an unrecoverable state, since the - migration loader then finds no estate section to retry from. With cleanup - last, a failure there instead leaves the estate data duplicated (already - live in the owner-scoped file, still present in the stale legacy + migrated, owner-scoped file. Setting it only *after* the owner-scoped write + is complete means a reader never observes an active owner whose file is not + yet populated. +- Owner-scoped writing and `set_active_owner` may fail, but both occur before + legacy removal, so the legacy section remains available for recovery. Once + the owner-scoped location is active, cleanup is the only failure tolerated: + it is deliberately last. Were the legacy section removed first, a failure + between that removal and the owner-scoped write would leave the estates in + neither place the CLI looks: the flat file no longer holds them, and no + active owner yet selects the owner-scoped file — an unrecoverable state, + since the migration loader then finds no estate section to retry from. With + cleanup last, a failure there instead leaves the estate data duplicated + (already live in the owner-scoped file, still present in the stale legacy section) — duplicated but reachable beats complete but invisible. Because the legacy file and the headline config are one and the same, step 2 -(`set_active_owner`) writes into the very file step 3 is about to edit. -Cleanup therefore reloads the file's current contents from disk +(`set_active_owner`) writes into the very file step 3 is about to edit. Cleanup +therefore reloads the file's current contents from disk (`_current_legacy_data`) rather than reusing the snapshot read before step 2: rewriting that earlier snapshot would silently erase the `github_owner` key -step 2 just wrote, and deleting the file outright (when the estate section -was its only original content) would discard the key entirely. +step 2 just wrote, and deleting the file outright (when the estate section was +its only original content) would discard the key entirely. ## Credentials @@ -145,11 +193,11 @@ order, from highest to lowest: `$XDG_CONFIG_HOME/concordat/owners//credentials.yaml`. `credential_environment` implements the lower two levels: it overlays the -process environment with values loaded from the owner's credentials file, -using `dict.setdefault` so an environment variable that is already set is -never overridden by the file. `concordat.cli._github_token_fallback` (and -equivalent per-command fallbacks) call this only when the CLI flag itself is -absent, giving the full three-level order. +process environment with values loaded from the owner's credentials file, using +`dict.setdefault` so an environment variable that is already set is never +overridden by the file. `concordat.cli._github_token_fallback` (and equivalent +per-command fallbacks) call this only when the CLI flag itself is absent, +giving the full three-level order. Only the names in `CREDENTIAL_KEYS` are honoured — `GITHUB_TOKEN`, `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, `AWS_SESSION_TOKEN`, @@ -159,17 +207,17 @@ Only the names in `CREDENTIAL_KEYS` are honoured — `GITHUB_TOKEN`, Two defensive details are worth knowing when working on this module: - **Group- or world-accessible files are refused, not read.** `load_credentials` - checks the file's mode bits before parsing it; any group or world - permission bit (including setuid/setgid) raises - `InsecureCredentialsError` rather than reading a file that might be - readable by other local users. The fix is `chmod 600`. + checks the file's mode bits before parsing it; any group or world permission + bit (including setuid/setgid) raises `InsecureCredentialsError` rather than + reading a file that might be readable by other local users. The fix is + `chmod 600`. - **Only genuine non-blank strings are honoured; non-string values are - dropped, not coerced.** `_recognized_credentials` requires both the key - and the value to be `str` instances, and the value to be non-blank after - stripping. A credential becomes an environment variable, so coercion would - be actively harmful: an empty `KEY:` would coerce to the literal string - `"None"`, and a YAML `false` would coerce to `"False"` — either handed to - a remote as though it were a real secret. Non-string values (a YAML `null` + dropped, not coerced.** `_recognized_credentials` requires both the key and + the value to be `str` instances, and the value to be non-blank after + stripping. A credential becomes an environment variable, so coercion would be + actively harmful: an empty `KEY:` would coerce to the literal string + `"None"`, and a YAML `false` would coerce to `"False"` — either handed to a + remote as though it were a real secret. Non-string values (a YAML `null` under a key, a boolean, a number) are therefore dropped rather than stringified. @@ -177,14 +225,14 @@ Concordat never writes this file; it is entirely operator-managed. ## API boundaries -Four modules define the layering between "what path does this data live -at" and "what does OpenTofu actually do with it": +Four modules define the layering between "what path does this data live at" and +"what does OpenTofu actually do with it": - **`concordat.credentials`** and **`concordat.xdg`** have no dependency on - git or provisioning code. `concordat.xdg` is pure path/config - arithmetic; `concordat.credentials` builds on it for owner resolution but - touches no git state. This keeps both modules importable from anywhere - else in the codebase without pulling in `pygit2`. + git or provisioning code. `concordat.xdg` is pure path/config arithmetic; + `concordat.credentials` builds on it for owner resolution but touches no git + state. This keeps both modules importable from anywhere else in the codebase + without pulling in `pygit2`. - **`concordat/estate_cache.py`** owns the git-backed cache of estate repositories. Two entry points are deliberately split by side effect: - `cache_destination(record, cache_directory=None)` is a **pure path @@ -200,42 +248,41 @@ at" and "what does OpenTofu actually do with it": it", not earlier. - **`concordat/estate_execution.py`** builds on `estate_cache` to run `tofu plan` / `tofu apply`. It wraps `ensure_estate_cache` so that - `EstateCacheError` surfaces to callers as `EstateExecutionError`, - keeping one error hierarchy per layer. `estate_workspace` is the - context manager that ties caching, temp-workspace cloning - (`clone_into_temp`), and cleanup together; it resolves the owner's XDG - state `runs/` directory the same way `estate_cache` resolves the owner's - cache directory, so kept workdirs (`--keep-workdir`) land somewhere - predictable. + `EstateCacheError` surfaces to callers as `EstateExecutionError`, keeping one + error hierarchy per layer. `estate_workspace` is the context manager that + ties caching, temp-workspace cloning (`clone_into_temp`), and cleanup + together; it resolves the owner's XDG state `runs/` directory the same way + `estate_cache` resolves the owner's cache directory, so kept workdirs + (`--keep-workdir`) land somewhere predictable. ### Estate module boundaries `concordat.estate` is the public façade for estate management; the modules -below sit beneath it, and `concordat.estate` imports each of them, never -the reverse: +below sit beneath it, and `concordat.estate` imports each of them, never the +reverse: - **`concordat/estate_config.py`** — configuration persistence and migration: loading and writing the owner-scoped estate configuration, the - legacy-flat migration (see [Legacy-flat - migration](#legacy-flat-migration)), and owner normalization. + legacy-flat migration (see [Legacy-flat migration](#legacy-flat-migration)), + and owner normalization. - **`concordat/estate_errors.py`** — the estate exception taxonomy. It is a - leaf module with no dependency on git or GitHub code, so any other layer - can import it without risking an import cycle. -- **`concordat/estate_git.py`** — git operations behind `concordat estate - init` and `concordat ls`: remote probing, inventory collection from a - clone, and template bootstrapping for a new estate. It knows nothing - about the GitHub API or the estate-init decision flow. + leaf module with no dependency on git or GitHub code, so any other layer can + import it without risking an import cycle. +- **`concordat/estate_git.py`** — git operations behind `concordat estate init` + and `concordat ls`: remote probing, inventory collection from a clone, and + template bootstrapping for a new estate. It knows nothing about the GitHub + API or the estate-init decision flow. - **`concordat/estate_github.py`** — the GitHub API calls concordat makes - when an estate repository must be created, and the translation of - github3's authentication failures into the estate error taxonomy. It - knows nothing about git or the estate-init decision flow. -- **`concordat/estate_repository.py`** — the *decisions* `concordat estate - init` makes (which owner an estate belongs to, whether its remote needs - provisioning), delegating the *how* to `estate_github` and `estate_git`. - Its imports are deliberate, not incidental: it is the single lookup site - the `concordat.estate` façade calls through, and the single seam the - test suite monkeypatches (see [Module-level monkeypatch - seams](#module-level-monkeypatch-seams)). + when an estate repository must be created, and the translation of github3's + authentication failures into the estate error taxonomy. It knows nothing + about git or the estate-init decision flow. +- **`concordat/estate_repository.py`** — the *decisions* + `concordat estate init` makes (which owner an estate belongs to, whether its + remote needs provisioning), delegating the *how* to `estate_github` and + `estate_git`. Its imports are deliberate, not incidental: it is the single + lookup site the `concordat.estate` façade calls through, and the single seam + the test suite monkeypatches (see + [Module-level monkeypatch seams](#module-level-monkeypatch-seams)). ## `concordat artefact rule run` @@ -244,27 +291,26 @@ rule-run subcommand exposed as `concordat artefact rule run `. ### The policy envelope -`build_envelope` (in `envelope.py`) assembles a `policy-input/ -rust-makefile-baseline` document (schema version 1) describing one local -checkout: whether a root `Cargo.toml` and `Makefile` exist, the parsed +`build_envelope` (in `envelope.py`) assembles a +`policy-input/ rust-makefile-baseline` document (schema version 1) describing +one local checkout: whether a root `Cargo.toml` and `Makefile` exist, the parsed `Cargo.toml` table (or `None`), and the validated `makeutil` report for the `Makefile` (or `None`). Root `Cargo.toml` presence is documented as *provisional* evidence of Rust applicability — the `.concordat` manifest -remains the eventual authority, per the module docstring, which points at -"the Parabellum ExecPlan decision log" for that decision. This document is -handed to Conftest as the input under audit. +remains the eventual authority, per the module docstring, which points at "the +Parabellum ExecPlan decision log" for that decision. This document is handed to +Conftest as the input under audit. ### Tool dependencies Two external tools must be on `PATH`: - **`makeutil`** (`concordat/rules/makefile_facts.py`) — the sole means by - which concordat inspects a `Makefile`; the module docstring states - plainly that "Concordat never parses GNU Make syntax itself". `makeutil - parse` is run with a 10-second default timeout, and its exit code - (0 = complete parse, 1 = recovered parse) must agree with the `parse.status` - field of its own JSON report, or the report is rejected as internally - inconsistent. + which concordat inspects a `Makefile`; the module docstring states plainly + that "Concordat never parses GNU Make syntax itself". `makeutil parse` is run + with a 10-second default timeout, and its exit code (0 = complete parse, 1 = + recovered parse) must agree with the `parse.status` field of its own JSON + report, or the report is rejected as internally inconsistent. - **`conftest`** (`concordat/rules/runner.py`) — evaluates the envelope against the rule package's Rego policy, with a 60-second timeout (`CONFTEST_TIMEOUT`). @@ -272,9 +318,9 @@ Two external tools must be on `PATH`: ### The `OperationalRuleError` contract `OperationalRuleError` (`concordat/errors.py`) is raised whenever rule -evaluation could not run at all — as distinct from a policy finding, which -is a successful evaluation that happens to report noncompliance. It carries -three pieces of context: +evaluation could not run at all — as distinct from a policy finding, which is a +successful evaluation that happens to report noncompliance. It carries three +pieces of context: - `operation` — a stable identifier for the failing action (for example `"load-rule-package"`, `"invoke-conftest"`, `"parse-cargo-toml"`). @@ -294,36 +340,34 @@ findings by `_overall_verdict`: could not prove compliance, and fails closed rather than passing). The `rule_run` CLI command maps these, plus operational failure, onto three -exit codes: `0` compliant, `1` at least one finding (including -indeterminate, which fails closed), `2` operational failure — an uncaught -`OperationalRuleError` is caught in `concordat.cli.main` and converted to -exit code 2, printed to standard error, distinct from the `1` that -`ConcordatError` maps to. +exit codes: `0` compliant, `1` at least one finding (including indeterminate, +which fails closed), `2` operational failure — an uncaught +`OperationalRuleError` is caught in `concordat.cli.main` and converted to exit +code 2, printed to standard error, distinct from the `1` that `ConcordatError` +maps to. ### Rule package identifier validation -Rule package identifiers are validated against a canonical pattern — -lower-case ASCII words joined by single hyphens -(`^[a-z0-9]+(?:-[a-z0-9]+)*$`) — **before** any filesystem access. -`_rule_package_dir` then joins the validated identifier to the packages -root and confirms the resolved path stays under that root even though the -pattern alone already excludes traversal characters; the containment check -exists so that a future loosening of the pattern cannot silently reach -outside the root. The root itself comes from `_rule_packages_dir()`, a -cached lookup performed on first use rather than at import, so a missing or -unreadable rule tree surfaces when a rule runs instead of when the module -is imported. +Rule package identifiers are validated against a canonical pattern — lower-case +ASCII words joined by single hyphens (`^[a-z0-9]+(?:-[a-z0-9]+)*$`) — +**before** any filesystem access. `_rule_package_dir` then joins the validated +identifier to the packages root and confirms the resolved path stays under that +root even though the pattern alone already excludes traversal characters; the +containment check exists so that a future loosening of the pattern cannot +silently reach outside the root. The root itself comes from +`_rule_packages_dir()`, a cached lookup performed on first use rather than at +import, so a missing or unreadable rule tree surfaces when a rule runs instead +of when the module is imported. ### Conftest exit codes -Only Conftest exit codes `0` (no policy failures) and `1` (policy failures) -are treated as policy verdicts; both are expected to emit a JSON result -document on stdout. Any other exit code — a malformed policy, a bad flag, a -missing input file — means Conftest did not evaluate the policy at all, even -if it printed something on stdout that looks like JSON. -`_require_policy_exit_code` rejects those with an `OperationalRuleError` -rather than risk decoding an operational failure as though it were a clean -run. +Only Conftest exit codes `0` (no policy failures) and `1` (policy failures) are +treated as policy verdicts; both are expected to emit a JSON result document on +stdout. Any other exit code — a malformed policy, a bad flag, a missing input +file — means Conftest did not evaluate the policy at all, even if it printed +something on stdout that looks like JSON. `_require_policy_exit_code` rejects +those with an `OperationalRuleError` rather than risk decoding an operational +failure as though it were a clean run. ### Packaging: installed package data and the source distribution @@ -332,24 +376,25 @@ The build backend is setuptools (`[build-system]` in `pyproject.toml`: `build-backend = "setuptools.build_meta"`). `[tool.setuptools] packages` lists every shipped package explicitly: -`concordat`, `concordat.auditor`, `concordat.persistence`, -`concordat.rules`, `concordat.canon`. The list is explicit rather than -`packages.find` because `concordat.canon` is an out-of-tree data package -that has to be named — that rules out `find`, so the in-tree subpackages -are listed alongside it rather than discovered automatically. - -`[tool.setuptools.package-dir]` maps `"concordat.canon" = -"platform-standards/canon"`, and `[tool.setuptools.package-data]` ships -`"concordat.canon" = ["lint-rules/**/*"]`. So the canon lint-rule tree -lands under `concordat/canon/lint-rules` in the wheel, which is the point: -`concordat artefact rule run` has to work from an installed wheel, not only -a source checkout. The runner resolves the rule-package tree through +`concordat`, `concordat.auditor`, `concordat.persistence`, `concordat.rules`, +`concordat.canon`. The list is explicit rather than `packages.find` because +`concordat.canon` is an out-of-tree data package that has to be named — that +rules out `find`, so the in-tree subpackages are listed alongside it rather +than discovered automatically. + +`[tool.setuptools.package-dir]` maps +`"concordat.canon" = "platform-standards/canon"`, and +`[tool.setuptools.package-data]` ships +`"concordat.canon" = ["lint-rules/**/*"]`. So the canon lint-rule tree lands +under `concordat/canon/lint-rules` in the wheel, which is the point: +`concordat artefact rule run` has to work from an installed wheel, not only a +source checkout. The runner resolves the rule-package tree through `importlib.resources`, with a source-checkout fallback — see `_rule_packages_dir()` above. `MANIFEST.in` controls the source distribution, and grafts three trees: -`platform-standards`, `scripts`, and `tests`. This preserves the sdist -contents the previous build backend shipped, per the file's own comment. +`platform-standards`, `scripts`, and `tests`. This preserves the sdist contents +the previous build backend shipped, per the file's own comment. ## Parabellum boundaries @@ -366,56 +411,55 @@ with an `owner` key and a `repositories` list of `{name, excluded?}` entries URL or a clone-directory path: - `_OWNER_PATTERN` — GitHub-owner shaped: alphanumerics and hyphens, no - leading/trailing hyphen, capped at 39 characters (GitHub's own owner - length limit). This is a stricter, length-bounded sibling of - `concordat.xdg`'s `_OWNER_PATTERN`, which has no length cap; the two are - independent patterns maintained separately, not a shared constant. + leading/trailing hyphen, capped at 39 characters (GitHub's own owner length + limit). This is a stricter, length-bounded sibling of `concordat.xdg`'s + `_OWNER_PATTERN`, which has no length cap; the two are independent patterns + maintained separately, not a shared constant. - `_REPO_NAME_PATTERN` — alphanumerics, dot, underscore, and hyphen, 1–100 - characters, with at least one non-dot character (so `.` and `..` cannot - be smuggled in as a "repository name" that later becomes a clone-directory + characters, with at least one non-dot character (so `.` and `..` cannot be + smuggled in as a "repository name" that later becomes a clone-directory component). -Both are checked again in `clone_and_audit` immediately before the values -are interpolated into a clone URL and a scratch-directory path, not only at +Both are checked again in `clone_and_audit` immediately before the values are +interpolated into a clone URL and a scratch-directory path, not only at manifest-load time — belt and braces for any caller reaching `clone_and_audit` directly rather than through `load_estate`. ### The append-only ledger and its idempotency rule -`docs/parabellum/ledger.jsonl` (by default) is an append-only JSON Lines -file: one JSON object per line, never rewritten or truncated -(`_append_record` opens the file in append mode and writes exactly one -record per call). Each record is durable — flushed to disk — before the -sweep moves on to the next repository, because auditing the whole estate -takes many minutes and clones over the network; an interrupted sweep resumes -from where the ledger left off rather than restarting. - -**Idempotency rule:** a repository is skipped, rather than re-audited, when -the ledger already holds a record for that repository at the same -`commit_sha` (`_already_ledgered`). Excluded entries use a variant of this -rule keyed on `verdict == "excluded"` instead of a commit, since an -exclusion has no commit to compare against. `--force` bypasses the -commit-based skip (but not the exclusion skip, which unconditionally -prevents duplicate exclusion records for the same repository). +`docs/parabellum/ledger.jsonl` (by default) is an append-only JSON Lines file: +one JSON object per line, never rewritten or truncated (`_append_record` opens +the file in append mode and writes exactly one record per call). Each record is +durable — flushed to disk — before the sweep moves on to the next repository, +because auditing the whole estate takes many minutes and clones over the +network; an interrupted sweep resumes from where the ledger left off rather +than restarting. + +**Idempotency rule:** a repository is skipped, rather than re-audited, when the +ledger already holds a record for that repository at the same `commit_sha` +(`_already_ledgered`). Excluded entries use a variant of this rule keyed on +`verdict == "excluded"` instead of a commit, since an exclusion has no commit +to compare against. `--force` bypasses the commit-based skip (but not the +exclusion skip, which unconditionally prevents duplicate exclusion records for +the same repository). ### The git boundary All git operations funnel through the module-private `_git` helper, which -shells out to the `git` binary (`subprocess.run`, fixed argv, no shell) with -a 300-second timeout (`GIT_TIMEOUT`). Every call site supplies an -`operation` and `resource` for the resulting `OperationalRuleError` if the -command is missing, times out, or exits non-zero. Two call sites build on -`_git`: +shells out to the `git` binary (`subprocess.run`, fixed argv, no shell) with a +300-second timeout (`GIT_TIMEOUT`). Every call site supplies an `operation` and +`resource` for the resulting `OperationalRuleError` if the command is missing, +times out, or exits non-zero. Two call sites build on `_git`: - `resolve_head(owner, name)` runs `git ls-remote HEAD` to obtain the default-branch head SHA **without cloning** — used to decide, cheaply, whether a repository has already been ledgered at its current head before paying for a clone. - `clone_and_audit(owner, name)` performs a shallow, single-branch clone - (`git clone --depth 1 --quiet`) into a temporary directory, resolves - `HEAD` there with `git rev-parse HEAD`, and hands the checkout to - `concordat.rules.run_rule` for the `rust-makefile-baseline` audit. The - sweep is audit-only: nothing here ever writes to an estate repository. + (`git clone --depth 1 --quiet`) into a temporary directory, resolves `HEAD` + there with `git rev-parse HEAD`, and hands the checkout to + `concordat.rules.run_rule` for the `rust-makefile-baseline` audit. The sweep + is audit-only: nothing here ever writes to an estate repository. ## Property tests and the bounded reachability contract @@ -423,10 +467,10 @@ command is missing, times out, or exits non-zero. Two call sites build on `tests/unit/test_properties.py` holds concordat's Hypothesis-based property tests. Its module docstring states the discipline the whole file follows: -"where a property restates a regex, it is written from the specification -rather than the implementation's pattern, so the two can disagree" — a -property test that reimplements the code under test proves nothing, so -each property is derived from the documented rule instead. The file covers: +"where a property restates a regex, it is written from the specification rather +than the implementation's pattern, so the two can disagree" — a property test +that reimplements the code under test proves nothing, so each property is +derived from the documented rule instead. The file covers: - **the owner-name grammar** (`TestOwnerNames`) — acceptance against `xdg.validate_owner` agrees with a grammar written independently of @@ -445,8 +489,8 @@ each property is derived from the documented rule instead. The file covers: append-only history, the latest record for a repository is the last one appended. -`test_a_component_joined_to_a_root_stays_inside_it` joins a generated name -to a real directory rather than a `tmp_path` fixture: Hypothesis rejects +`test_a_component_joined_to_a_root_stays_inside_it` joins a generated name to a +real directory rather than a `tmp_path` fixture: Hypothesis rejects function-scoped fixtures, since they would be created once and then shared across every generated example rather than being fresh per example. @@ -456,21 +500,20 @@ The rule package's `policy/rust_makefile_baseline_test.rego`, under the `-- bounded reachability contract --` banner, enumerates `lint` prerequisite chains of increasing depth over one envelope. In the shipped `rust-makefile-baseline` v0.2.0 rule package, QG-001 proves gate delegation -within one prerequisite hop, so this suite pins the boundary between -"provable" and "indeterminate" rather than sampling it: depth 0 (a direct -gate invocation) and depth 1 (one hop of delegation) are compliant, and -every deeper chain is indeterminate. This one-hop bound is the semantics of -the shipped v0.2.0 rule package only. `docs/concordat-design.md` §2.2.1 -specifies, but has not shipped, a v0.3.0 that widens QG-001's delegation -proof from one prerequisite hop to a full static closure over the parsed -Makefile: the closure's edges are a rule's prerequisites plus any recipe -line invoking `$(MAKE) ` in the same file. That closure is -cycle-safe and needs no depth bound because every edge is a fact from the -single parsed file; dynamic edges (`$(MAKE) $(VAR)`, `$(MAKE) -C`, recursive -make into other files) and includes stay indeterminate. Under v0.3.0 the -`two_hop` fixture's expectation changes from indeterminate to compliant. -`build` and `test` targets are kept present in every case, so FP-003 stays -silent and QG-001 is the only variable under test. +within one prerequisite hop, so this suite pins the boundary between "provable" +and "indeterminate" rather than sampling it: depth 0 (a direct gate invocation) +and depth 1 (one hop of delegation) are compliant, and every deeper chain is +indeterminate. This one-hop bound is the semantics of the shipped v0.2.0 rule +package only. `docs/concordat-design.md` §2.2.1 specifies, but has not shipped, +a v0.3.0 that widens QG-001's delegation proof from one prerequisite hop to a +full static closure over the parsed Makefile: the closure's edges are a rule's +prerequisites plus any recipe line invoking `$(MAKE) ` in the +same file. That closure is cycle-safe and needs no depth bound because every +edge is a fact from the single parsed file; dynamic edges (`$(MAKE) $(VAR)`, +`$(MAKE) -C`, recursive make into other files) and includes stay indeterminate. +Under v0.3.0 the `two_hop` fixture's expectation changes from indeterminate to +compliant. `build` and `test` targets are kept present in every case, so FP-003 +stays silent and QG-001 is the only variable under test. This policy suite is not wired into the Makefile. It is run directly with Conftest: @@ -482,16 +525,16 @@ conftest verify --policy policy --data fixtures/data.json ## Test seams and subprocess contracts -The suite substitutes real subprocesses and network access with two -distinct mechanisms, depending on whether the code under test shells out -directly or calls another concordat function that does. +The suite substitutes real subprocesses and network access with two distinct +mechanisms, depending on whether the code under test shells out directly or +calls another concordat function that does. ### `cmd_mox`: the subprocess-mocking harness `tests/conftest.py` defines a small, purpose-built `CmdMox` harness (not the similarly-named third-party `cmdmox` library) and exposes it as the `cmd_mox` -pytest fixture. It monkeypatches `subprocess.run` globally for the duration -of a test (`CmdMox.replay`), so it intercepts *any* subprocess invocation — +pytest fixture. It monkeypatches `subprocess.run` globally for the duration of +a test (`CmdMox.replay`), so it intercepts *any* subprocess invocation — `makeutil`, `conftest`, `git` — regardless of which module issued it. Expectations are queued with a fluent builder: @@ -503,31 +546,31 @@ cmd_mox.mock("conftest").with_args("test", "--policy", ...).returns( Each queued expectation is consumed in order (`collections.deque`); an unexpected command, a command-name mismatch, or an argument mismatch raises -immediately, and any expectations left unconsumed at the end of a test raise -via `CmdMox.verify`. +immediately, and any expectations left unconsumed at the end of a test raise via +`CmdMox.verify`. ### Module-level monkeypatch seams Where concordat code calls another concordat function directly (rather than shelling out), tests patch that function on the module attribute the caller -actually resolves at call time — which is not always the function's -*defining* module: +actually resolves at call time — which is not always the function's *defining* +module: - **`concordat.estate_repository._probe_remote`** — `estate_repository.py` imports `_probe_remote` from `concordat.estate_git` and calls it as a bare name, so patching `concordat.estate_git._probe_remote` would leave - `estate_repository`'s already-bound reference untouched. A comment above - the import states explicitly that tests must patch + `estate_repository`'s already-bound reference untouched. A comment above the + import states explicitly that tests must patch `estate_repository._probe_remote` (along with `._build_client` and - `._create_repository`) — the name as it appears in `estate_repository`'s - own namespace. + `._create_repository`) — the name as it appears in `estate_repository`'s own + namespace. - **`scripts.parabellum_sweep.resolve_head`** — `resolve_head` is defined - directly in `parabellum_sweep.py`, so there is no import indirection to - worry about: tests import the module (commonly aliased `sweep`) and - monkeypatch `sweep.resolve_head` directly, replacing the network-touching - `git ls-remote` call with a fixed SHA or a function that raises - `OperationalRuleError`, without needing `cmd_mox` at all. + directly in `parabellum_sweep.py`, so there is no import indirection to worry + about: tests import the module (commonly aliased `sweep`) and monkeypatch + `sweep.resolve_head` directly, replacing the network-touching `git ls-remote` + call with a fixed SHA or a function that raises `OperationalRuleError`, + without needing `cmd_mox` at all. The rule of thumb: if the code shells out via `subprocess.run`, reach for -`cmd_mox`; if it calls a sibling concordat function, monkeypatch that -function on the module that does the calling. +`cmd_mox`; if it calls a sibling concordat function, monkeypatch that function +on the module that does the calling. diff --git a/docs/execplans/parabellum-vertical-slice.md b/docs/execplans/parabellum-vertical-slice.md index a465af9..d54eb3f 100644 --- a/docs/execplans/parabellum-vertical-slice.md +++ b/docs/execplans/parabellum-vertical-slice.md @@ -793,9 +793,11 @@ Python signatures that must exist at the end of Milestone D: # concordat/rules/makefile_facts.py def inspect_makefile(path: Path, *, timeout: float = 10.0) -> MakefileFacts: ... + # concordat/rules/envelope.py def build_envelope(checkout: Path) -> dict[str, object]: ... + # concordat/rules/runner.py def run_rule(rule_id: str, checkout: Path) -> RuleRunResult: ... ``` @@ -804,9 +806,9 @@ def run_rule(rule_id: str, checkout: Path) -> RuleRunResult: ... `RuleRunResult` carries `verdict`, `findings`, and `exit_code`. Rendering is separate from evaluation: `run_rule` returns the structured -result, and two standalone renderers in `concordat/rules/runner.py` format -it — the choice of output format is the CLI's concern, not the evaluator's, -so `run_rule` takes no formatting parameter. +result, and two standalone renderers in `concordat/rules/runner.py` format it — +the choice of output format is the CLI's concern, not the evaluator's, so +`run_rule` takes no formatting parameter. ```python # concordat/rules/runner.py diff --git a/docs/local-validation-of-github-actions-with-act-and-pytest.md b/docs/local-validation-of-github-actions-with-act-and-pytest.md index 7073243..2bad952 100644 --- a/docs/local-validation-of-github-actions-with-act-and-pytest.md +++ b/docs/local-validation-of-github-actions-with-act-and-pytest.md @@ -174,6 +174,7 @@ loop: ```python from cmd_mox import CmdMox + def test_record(tmp_path: Path) -> None: artifact_dir = tmp_path / "act-artifacts" with CmdMox() as mox: @@ -183,7 +184,6 @@ def test_record(tmp_path: Path) -> None: assert code == 0, logs mox.verify() assert gh.call_count == 1 - ``` - **Replay** deterministically with mocks. Configure expectations using the @@ -202,7 +202,6 @@ def test_replay(tmp_path: Path, cmd_mox) -> None: code, _, logs = run_act(artifact_dir=artifact_dir) assert code == 0, logs cmd_mox.verify() - ``` ## Concordat Auditor workflow harness diff --git a/docs/roadmap.md b/docs/roadmap.md index 52b6ee2..da15938 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -73,8 +73,8 @@ template tree into published platform-standards repositories. (`rust-makefile-baseline`, audit-only) exists with `rule.yaml`, a Rego sensor, fixtures, and policy tests, and `concordat artefact rule run --repo PATH --format table` (or - `--format json`) evaluates it against a local checkout (Operation - Parabellum vertical slice; `docs/execplans/parabellum-vertical-slice.md`). + `--format json`) evaluates it against a local checkout (Operation Parabellum + vertical slice; `docs/execplans/parabellum-vertical-slice.md`). `rule validate` and the mutation vocabulary remain open. ### 1.3. Ship the estate execution CLI diff --git a/docs/scripting-standards.md b/docs/scripting-standards.md index f50f8ea..e02376f 100644 --- a/docs/scripting-standards.md +++ b/docs/scripting-standards.md @@ -100,16 +100,16 @@ def main( # Required parameters bin_name: Annotated[str, Parameter(required=True)], version: Annotated[str, Parameter(required=True)], - # Optional scalars package_name: Optional[str] = None, target: Optional[str] = None, outdir: Optional[Path] = None, dry_run: bool = False, - # Lists (whitespace/newline separated by default) formats: list[str] | None = None, - man_paths: Annotated[list[Path] | None, Parameter(env_var="INPUT_MAN_PATHS")] = None, + man_paths: Annotated[ + list[Path] | None, Parameter(env_var="INPUT_MAN_PATHS") + ] = None, deb_depends: list[str] | None = None, rpm_depends: list[str] | None = None, ): @@ -257,7 +257,9 @@ f.write_text("1.2.3\n", encoding="utf-8") version = f.read_text(encoding="utf-8").strip() # Atomic write pattern (tmp → replace) -with tempfile.NamedTemporaryFile("w", delete=False, dir=f.parent, encoding="utf-8") as tmp: +with tempfile.NamedTemporaryFile( + "w", delete=False, dir=f.parent, encoding="utf-8" +) as tmp: tmp.write("new-contents\n") tmp_path = Path(tmp.name) @@ -302,6 +304,7 @@ from plumbum.cmd import git app = App(config=cyclopts.config.Env("INPUT_", command=False)) + @app.default def main( *, @@ -326,6 +329,7 @@ def main( "dist": str(dist), }) + if __name__ == "__main__": app() ``` diff --git a/docs/users-guide.md b/docs/users-guide.md index efc1c42..e1a0b41 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1,9 +1,8 @@ # concordat Users' Guide -For the internal module boundaries behind this CLI — the XDG layout, -credential resolution order, cache/execution API split, and the rule-run and -Parabellum sweep contracts — see -[`docs/developers-guide.md`](developers-guide.md). +For the internal module boundaries behind this CLI — the XDG layout, credential +resolution order, cache/execution API split, and the rule-run and Parabellum +sweep contracts — see [`docs/developers-guide.md`](developers-guide.md). ## Overview @@ -24,6 +23,14 @@ workflows read the same flag before applying changes. 2. Invoke the CLI with `uv run` to ensure the correct environment is used. +## Runtime implementation + +The public `concordat.hello` entry point uses the optional Rust implementation +when `_concordat_rs` is available and falls back to the pure-Python +implementation when that extension is absent. If importing the extension raises +`ModuleNotFoundError` for another module or dependency, that exception is raised +to the caller rather than being mistaken for a missing extension. + ## Enrolling repositories - Enrol one or more repositories by passing their paths: @@ -121,12 +128,11 @@ workflows read the same flag before applying changes. ## Configuration, credentials, cache, and state locations -Concordat's local configuration, credentials, caches, and state live under -the XDG base directories. A single global headline file names the active -configured owner; per-owner configuration, credentials, estate caches, and -state are namespaced beneath that owner, while the OpenTofu provider plugin -cache is shared across owners, since provider binaries are identical -regardless of owner: +Concordat's local configuration, credentials, caches, and state live under the +XDG base directories. A single global headline file names the active configured +owner; per-owner configuration, credentials, estate caches, and state are +namespaced beneath that owner, while the OpenTofu provider plugin cache is +shared across owners, since provider binaries are identical regardless of owner: - `$XDG_CONFIG_HOME/concordat/config.yaml` — the **headline** config, global rather than owner-namespaced; its `github_owner` key names the active owner. @@ -319,9 +325,9 @@ without leaving the CLI. Both commands require `GITHUB_TOKEN` and the estate's `SCW_SECRET_KEY` or `SPACES_ACCESS_KEY_ID`/`SPACES_SECRET_ACCESS_KEY` onto `AWS_ACCESS_KEY_ID`/`AWS_SECRET_ACCESS_KEY` when needed, and fails fast if none of the pairs is present. Standard error (stderr) logs echo the backend - bucket, key, region, and config path—never credentials—for traceability. - If the manifest is absent or `enabled: false`, `plan` and `apply` keep - using the local state layout. + bucket, key, region, and config path—never credentials—for traceability. If + the manifest is absent or `enabled: false`, `plan` and `apply` keep using the + local state layout. - Reconcile the estate with `concordat apply`. The command requires an explicit `--auto-approve` to match OpenTofu's automation guard. @@ -554,17 +560,16 @@ with the following measures: unreadable, and leaking it defeats the encryption. - **Client-side or envelope encryption (optional):** a genuinely separate, independent control from SSE-C, useful whichever provider is in use. Wrap - `tofu state`/`tofu plan` calls with tooling that encrypts state before - upload. + `tofu state`/`tofu plan` calls with tooling that encrypts state before upload. - **Audit access logs:** Periodically review bucket access logs to detect unauthorized reads or unexpected access patterns. ### Estate configuration file Concordat stores estate metadata in -`$XDG_CONFIG_HOME/concordat/owners//config.yaml`, where `` is -the active owner configured in the headline configuration. The file is -regular YAML 1.2 with an `estate` section: +`$XDG_CONFIG_HOME/concordat/owners//config.yaml`, where `` is the +active owner configured in the headline configuration. The file is regular YAML +1.2 with an `estate` section: ```yaml estate: diff --git a/pyproject.toml b/pyproject.toml index de26a4d..976ec02 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -34,6 +34,7 @@ dev = [ "betamax", "hypothesis", "textual>=8.2.8,<9.0", + "df12-python-lints @ git+https://github.com/leynos/df12-python-lints.git@9c835f35b0f1690597ade799c9c6a30bc5922959", ] [tool.pyright] @@ -43,6 +44,8 @@ include = ["concordat"] [tool.ruff] line-length = 88 +preview = true +target-version = "py313" [tool.ruff.lint] select = [ @@ -77,11 +80,20 @@ select = [ "PERF", "TRY", "D", + "DOC", + "ASYNC", "ANN", ] ignore = [ - "D203", - "D213", + "D203", # Conflicts with D211; keep class summaries adjacent to the class. + "D213", # Conflicts with D212; keep summaries on the first docstring line. + # S404 only reports importing subprocess. Every process boundary remains + # subject to S603/S607, whose call-site checks establish the real security + # invariant: fixed argv and an intentional executable-resolution policy. + "S404", + # These stateful test doubles model a command protocol, not passive data; + # converting them to dataclasses would obscure the behaviour under test. + "B903", ] # `assert` is the point of a test, and of the assertion helpers that test # modules share; the support pattern covers helper modules whose names cannot @@ -114,6 +126,150 @@ datetime = "dt" "msgspec.json" = "msjson" typing = "typ" +[tool.ruff.lint.pydocstyle] +convention = "numpy" + +[tool.ruff.lint.pydoclint] +ignore-one-line-docstrings = true + +[tool.pylint.main] +recursive = true +max-module-lines = 800 + +[tool.pylint.design] +# CLI commands, BDD steps, and GitHub callbacks carry public parameter +# contracts. Ten preserves those boundaries while flagging unwieldy internals. +max-args = 10 +max-locals = 20 +max-statements = 70 +max-positional-arguments = 10 +# The policy dispatcher deliberately exposes the complete audit matrix in one +# place; splitting its branches would obscure that relationship. +max-branches = 15 + +[tool.pylint."messages control"] +disable = [ + "all", + # Managed PyPy can lag the project's supported CPython syntax. Keep the + # baseline pass useful on files it can parse. + "syntax-error", +] +enable = [ + # Logging format safety. + "logging-format-interpolation", + "logging-format-truncated", + "logging-fstring-interpolation", + "logging-not-lazy", + "logging-too-few-args", + "logging-too-many-args", + "logging-unsupported-format", + + # Pattern-matching correctness and readability. + "bare-name-capture-pattern", + "invalid-match-args-definition", + "match-class-bind-self", + "match-class-positional-attributes", + "multiple-class-sub-patterns", + "too-many-positional-sub-patterns", + + # Control-flow simplification. + "chained-comparison", + "condition-evals-to-constant", + "consider-merging-isinstance", + "consider-swap-variables", + "consider-using-in", + "consider-using-max-builtin", + "consider-using-min-builtin", + "consider-using-sys-exit", + "consider-using-ternary", + "inconsistent-return-statements", + "no-else-break", + "no-else-continue", + "no-else-raise", + "no-else-return", + "redefined-argument-from-local", + "simplifiable-condition", + "simplifiable-if-expression", + "simplifiable-if-statement", + "simplify-boolean-expression", + "stop-iteration-return", + "super-with-arguments", + "trailing-comma-tuple", + "unnecessary-negation", + "useless-return", + + # Collection and iterator idioms. + "consider-iterating-dictionary", + "consider-using-dict-comprehension", + "consider-using-dict-items", + "consider-using-enumerate", + "consider-using-f-string", + "consider-using-generator", + "consider-using-get", + "consider-using-join", + "consider-using-set-comprehension", + "unnecessary-comprehension", + "unnecessary-dict-index-lookup", + "unnecessary-list-index-lookup", + "use-a-generator", + "use-dict-literal", + "use-implicit-booleaness-not-comparison", + "use-implicit-booleaness-not-comparison-to-string", + "use-implicit-booleaness-not-len", + "use-list-literal", + "use-maxsplit-arg", + "use-sequence-for-iteration", + "use-yield-from", + + # Runtime APIs, resources, and compatibility hazards. + "bad-open-mode", + "bad-thread-instantiation", + "boolean-datetime", + "consider-using-with", + "deprecated-argument", + "deprecated-attribute", + "deprecated-class", + "deprecated-decorator", + "deprecated-method", + "forgotten-debug-statement", + "invalid-envvar-default", + "invalid-envvar-value", + "method-cache-max-size-none", + "redundant-unittest-assert", + "shallow-copy-environ", + "singledispatch-method", + "singledispatchmethod-function", + "subprocess-popen-preexec-fn", + "subprocess-run-check", + "unnecessary-dunder-call", + "unnecessary-ellipsis", + "unspecified-encoding", + + # Source layout and text hygiene. + "missing-final-newline", + "mixed-line-endings", + "superfluous-parens", + "trailing-newlines", + "trailing-whitespace", + "unexpected-line-ending-format", + + # Mutation during iteration. + "modified-iterating-dict", + "modified-iterating-list", + "modified-iterating-set", + + # Structural complexity limits. + "too-many-arguments", + "too-many-boolean-expressions", + "too-many-branches", + "too-many-lines", + "too-many-locals", + "too-many-nested-blocks", + "too-many-positional-arguments", + "too-many-public-methods", + "too-many-statements", +] + [tool.pytest.ini_options] # Ensure asyncio fixtures create a new event loop for each test asyncio_default_fixture_loop_scope = "function" diff --git a/scripts/canon_artifacts.py b/scripts/canon_artifacts.py index e7dbd5b..0eb045a 100644 --- a/scripts/canon_artifacts.py +++ b/scripts/canon_artifacts.py @@ -5,13 +5,11 @@ outdated or missing artifacts. """ -# ruff: noqa: TRY003 - from __future__ import annotations import dataclasses -from pathlib import Path # noqa: TC003 -from typing import Annotated # noqa: ICN003 +import typing as typ +from pathlib import Path # noqa: TC003 # Runtime annotations require this import. from cyclopts import App, Parameter @@ -94,7 +92,7 @@ def list_artifacts( @app.command() def status( - config: Annotated[StatusConfig, Parameter(name="*")], + config: typ.Annotated[StatusConfig, Parameter(name="*")], ) -> int: """Print a table comparing published artifacts against the template.""" return _render_status(config) @@ -169,12 +167,7 @@ def _determine_sync_ids( config: CliSyncConfig, comparisons: list[ArtifactComparison], ) -> set[str]: - """Determine which artifact IDs to sync based on configuration. - - When `all_outdated` is set but no artifacts match the caller's filters (for - example, `--types` yields an empty comparison set), this returns an empty - set to indicate a no-op rather than raising. - """ + """Determine artifact IDs to sync, treating filtered selections as no-ops.""" if config.all_outdated: if not comparisons: return set() @@ -185,14 +178,14 @@ def _determine_sync_ids( } if config.artifact_ids: return set(config.artifact_ids) - raise CanonArtifactsError( + raise CanonArtifactsError( # noqa: TRY003 # Domain error provides operator remediation. "No artifacts selected for sync. Pass explicit IDs or use --all-outdated." ) @app.command() def sync( - config: Annotated[CliSyncConfig, Parameter(name="*")], + config: typ.Annotated[CliSyncConfig, Parameter(name="*")], ) -> int: """Copy template artifacts into the published checkout.""" published_root = config.published_root.resolve() @@ -242,7 +235,7 @@ def tui( try: from scripts.canon_artifacts_tui import CanonArtifactsApp except ModuleNotFoundError as exc: # pragma: no cover - raise CanonArtifactsError( + raise CanonArtifactsError( # noqa: TRY003 # Domain error provides operator remediation. "Textual is required for `tui`. Install dev dependencies via " "`make build` or `uv sync --group dev`." ) from exc diff --git a/scripts/canon_artifacts_tui.py b/scripts/canon_artifacts_tui.py index fd621a2..7f18184 100644 --- a/scripts/canon_artifacts_tui.py +++ b/scripts/canon_artifacts_tui.py @@ -57,11 +57,7 @@ def compose(self) -> ComposeResult: def on_mount(self) -> None: """Populate the table on startup.""" self._table.add_columns("id", "type", "status", "template", "published", "path") - self._refresh() - - def action_refresh(self) -> None: - """Reload the manifest comparison and update the table.""" - self._refresh() + self.action_refresh() def action_sync_selected(self) -> None: """Sync the currently highlighted artifact when it is missing/outdated.""" @@ -82,7 +78,7 @@ def action_sync_selected(self) -> None: ids={comparison.id}, ), ) - self._refresh() + self.action_refresh() def action_sync_all_outdated(self) -> None: """Sync every missing/outdated artifact currently shown.""" @@ -101,10 +97,14 @@ def action_sync_all_outdated(self) -> None: ids=outdated_ids, ), ) - self._refresh() + self.action_refresh() + + def action_refresh(self) -> None: + """Recompute comparisons and rewrite the table rows in place. - def _refresh(self) -> None: - """Recompute comparisons and rewrite the table rows.""" + The comparison state is refreshed from the manifest and published + checkout, then the existing table is cleared and repopulated. + """ comparisons = list( compare_manifest_to_published( self._manifest, diff --git a/scripts/canon_workflows.py b/scripts/canon_workflows.py index 0b55df5..769c43e 100644 --- a/scripts/canon_workflows.py +++ b/scripts/canon_workflows.py @@ -16,7 +16,7 @@ ACT_ARCH = "linux/amd64" -@dataclasses.dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class WorkflowMeta: """Describe the location of a canonical workflow and its sample event.""" @@ -134,7 +134,7 @@ def run(name: str, *, dry_run: bool = False) -> None: return _act_available() - subprocess.run(args, check=True) # noqa: S603 + subprocess.run(args, check=True) # noqa: S603 # Fixed argv, no shell. def main() -> None: # pragma: no cover - exercised via CLI diff --git a/scripts/parabellum_ledger.py b/scripts/parabellum_ledger.py index 2ec3d34..47ae35e 100644 --- a/scripts/parabellum_ledger.py +++ b/scripts/parabellum_ledger.py @@ -139,15 +139,25 @@ def _ledger_record( The ledger is append-only and read back on every sweep, so a truncated or hand-edited line has to surface as an operational error rather than be trusted into the typed flow by a cast. + + Returns + ------- + LedgerRecord + The validated decoded ledger record. + + Raises + ------ + OperationalRuleError + If the decoded line is not a valid ledger record. """ if not isinstance(decoded, dict): message = f"ledger {path} line {line_number} is not a JSON object" - raise _ledger_error(message, path) + raise OperationalRuleError(message, operation="load-ledger", resource=path) record = typ.cast("dict[str, object]", decoded) missing = sorted(_LEDGER_REQUIRED_KEYS - record.keys()) if missing: message = f"ledger {path} line {line_number} is missing {missing}" - raise _ledger_error(message, path) + raise OperationalRuleError(message, operation="load-ledger", resource=path) _validate_record_fields(record, path, line_number) return typ.cast("LedgerRecord", record) @@ -184,6 +194,11 @@ def _matches_expected_type(value: object, expected: type) -> bool: `bool` is excluded from the integer check: it subclasses `int`, so `true` would otherwise pass as a schema version or a line number. + + Returns + ------- + bool + Whether *value* has the expected type. """ if expected is int: if isinstance(value, bool): @@ -291,7 +306,8 @@ def _load_ledger(path: pathlib.Path) -> Ledger: def _timestamp() -> str: return ( - dt.datetime.now(dt.UTC) + dt.datetime + .now(dt.UTC) .replace(microsecond=0) .isoformat() .replace("+00:00", "Z") @@ -341,6 +357,11 @@ def _finding_record(finding: Finding) -> FindingRecord: `dataclasses.asdict` returns `dict[str, Any]`, which would defeat the point of the record types. + + Returns + ------- + FindingRecord + The typed serialized representation of *finding*. """ return { "rule_id": finding.rule_id, diff --git a/scripts/parabellum_manifest.py b/scripts/parabellum_manifest.py index 113e208..7f075f5 100644 --- a/scripts/parabellum_manifest.py +++ b/scripts/parabellum_manifest.py @@ -75,6 +75,16 @@ def _validated_identifier( Names from the manifest become URL segments and clone-directory components, so they are validated here, at the boundary, rather than trusted downstream. + + Returns + ------- + str + The validated identifier. + + Raises + ------ + OperationalRuleError + If *value* does not match *pattern*. """ if isinstance(value, str) and pattern.fullmatch(value): return value @@ -129,11 +139,23 @@ def _repository_entries( Only a list is a repository collection. A scalar raises on iteration and a bare string is walked character by character, so both are refused here rather than reaching the entry decoding as `TypeError`. + + Returns + ------- + tuple[EstateEntry, ...] + The decoded repository entries. + + Raises + ------ + OperationalRuleError + If *repositories* is not a list. """ if not isinstance(repositories, list): - raise _manifest_error( - path, - f"has a `repositories` value that is not a list: {repositories!r}", + raise OperationalRuleError( # noqa: TRY003 # Domain error identifies the malformed manifest. + f"estate manifest {path} has a `repositories` value that is not " + f"a list: {repositories!r}", + operation="load-estate-manifest", + resource=path, ) return tuple(_repository_entry(item, path) for item in repositories) @@ -146,6 +168,9 @@ def load_estate(path: pathlib.Path) -> Estate: operational error naming the manifest, not as a `TypeError` from a subscript. + Name validation uses ``_validated_identifier`` and reports its own + ``OperationalRuleError`` when an owner or repository name is invalid. + Parameters ---------- path: @@ -162,19 +187,23 @@ def load_estate(path: pathlib.Path) -> Estate: With ``operation="load-estate-manifest"``, when the document is not a mapping, a required key (``repositories`` or ``owner``) is missing, the ``repositories`` value is not a list, a repository - entry is malformed, or an owner or repository name fails its - validation pattern. + entry is malformed. """ document = YAML(typ="safe").load(path.read_text(encoding="utf-8")) if not isinstance(document, dict): - raise _manifest_error( - path, - f"is not a mapping: {type(document).__name__}", + raise OperationalRuleError( # noqa: TRY003 # Domain error identifies the malformed manifest. + f"estate manifest {path} is not a mapping: {type(document).__name__}", + operation="load-estate-manifest", + resource=path, ) for key in ("repositories", "owner"): if key not in document: - raise _manifest_error(path, f"is missing key {key!r}") + raise OperationalRuleError( # noqa: TRY003 # Domain error identifies the malformed manifest. + f"estate manifest {path} is missing key {key!r}", + operation="load-estate-manifest", + resource=path, + ) entries = _repository_entries(document["repositories"], path) owner = _validated_identifier(document["owner"], _OWNER_PATTERN, "owner", path) return Estate(owner=owner, repositories=entries) diff --git a/scripts/parabellum_report.py b/scripts/parabellum_report.py index 0167aef..bbfa4ff 100644 --- a/scripts/parabellum_report.py +++ b/scripts/parabellum_report.py @@ -106,8 +106,10 @@ def render_report(ledger_path: pathlib.Path = DEFAULT_LEDGER_PATH) -> str: "Generated from `docs/parabellum/ledger.jsonl` by", "`python -m scripts.parabellum_sweep report`. Do not edit by hand.", "", - f"Rule package: `{RULE_PACKAGE}` v{RULE_VERSION}; " - f"makeutil `{MAKEUTIL_REV[:12]}`.", + ( + f"Rule package: `{RULE_PACKAGE}` v{RULE_VERSION}; " + f"makeutil `{MAKEUTIL_REV[:12]}`." + ), "", "## Summary", "", @@ -121,17 +123,15 @@ def render_report(ledger_path: pathlib.Path = DEFAULT_LEDGER_PATH) -> str: lines.extend( f"- {_cell(rule_id)}: {rule_counts[rule_id]}" for rule_id in sorted(rule_counts) ) - lines.extend( - [ - "", - "## Repositories", - "", - "Table 1: Latest verdict and findings per estate repository.", - "", - "| Repository | Verdict | Commit | Findings |", - "| ---------- | ------- | ------ | -------- |", - ] - ) + lines.extend([ + "", + "## Repositories", + "", + "Table 1: Latest verdict and findings per estate repository.", + "", + "| Repository | Verdict | Commit | Findings |", + "| ---------- | ------- | ------ | -------- |", + ]) for repository in sorted(latest): record = latest[repository] commit = (record["commit_sha"] or "")[:12] diff --git a/scripts/parabellum_sweep.py b/scripts/parabellum_sweep.py index 7e6595f..3eec99c 100644 --- a/scripts/parabellum_sweep.py +++ b/scripts/parabellum_sweep.py @@ -320,7 +320,7 @@ def _audit_record(owner: str, entry: EstateEntry) -> LedgerRecord: return record -@dataclasses.dataclass +@dataclasses.dataclass(slots=True) class _SweepSession: """Mutable per-invocation state for one estate sweep. @@ -385,11 +385,7 @@ def _process_auditable(self, entry: EstateEntry) -> bool: return False def _sweep_auditable_entry(self, entry: EstateEntry) -> bool: - """Audit one non-excluded entry and report audit-slot consumption. - - A head-resolution failure and a completed audit both consume an audit - slot; an idempotent skip of an already-ledgered commit does not. - """ + """Process one auditable entry and report whether it consumed an audit slot.""" repository = f"{self.owner}/{entry.name}" try: head = resolve_head(self.owner, entry.name) @@ -420,6 +416,20 @@ def run_sweep( """Sweep the estate and append new records to the ledger. Returns the records appended by this invocation. + + Parameters + ---------- + estate_path : pathlib.Path + Path to the estate manifest. + ledger_path : pathlib.Path + Path to the append-only campaign ledger. + options : SweepOptions + Filters and execution options for the sweep. + + Returns + ------- + Ledger + Records appended during this invocation. """ estate = load_estate(estate_path) session = _SweepSession( @@ -457,6 +467,16 @@ def sweep_command(options: SweepCommandOptions = _DEFAULT_COMMAND_OPTIONS) -> in ``--only`` takes a comma-separated list of repository names; ``--limit`` bounds how many repositories are audited this run. + + Parameters + ---------- + options : SweepCommandOptions + Parsed command-line options controlling the sweep. + + Returns + ------- + int + Zero after the sweep completes successfully. """ only_set = ( {name.strip() for name in options.only.split(",") if name.strip()} diff --git a/scripts/tests/test_parabellum_cli.py b/scripts/tests/test_parabellum_cli.py index e136789..78bbcc1 100644 --- a/scripts/tests/test_parabellum_cli.py +++ b/scripts/tests/test_parabellum_cli.py @@ -48,13 +48,11 @@ def fake_run_sweep( ledger_path: pathlib.Path, options: sweep.SweepOptions, ) -> sweep.Ledger: - calls.append( - { - "estate_path": estate_path, - "ledger_path": ledger_path, - "options": options, - } - ) + calls.append({ + "estate_path": estate_path, + "ledger_path": ledger_path, + "options": options, + }) return [] monkeypatch.setattr(sweep, "run_sweep", fake_run_sweep) @@ -78,19 +76,17 @@ def test_flags_reach_run_sweep( """Every top-level flag is parsed and forwarded unchanged.""" calls = self._capture(monkeypatch) - exit_code = self._invoke( - [ - "--only", - "statelet", - "--limit", - "1", - "--force", - "--estate", - str(estate_path), - "--ledger", - str(ledger_path), - ] - ) + exit_code = self._invoke([ + "--only", + "statelet", + "--limit", + "1", + "--force", + "--estate", + str(estate_path), + "--ledger", + str(ledger_path), + ]) assert exit_code == 0, exit_code assert len(calls) == 1, calls @@ -133,16 +129,14 @@ def test_only_flag_is_parsed_into_a_filter( """ calls = self._capture(monkeypatch) - self._invoke( - [ - "--only", - only, - "--estate", - str(tmp_path / "estate.yaml"), - "--ledger", - str(tmp_path / "ledger.jsonl"), - ] - ) + self._invoke([ + "--only", + only, + "--estate", + str(tmp_path / "estate.yaml"), + "--ledger", + str(tmp_path / "ledger.jsonl"), + ]) assert len(calls) == 1, calls assert calls[0]["options"].only == expected, calls[0] @@ -176,14 +170,12 @@ def test_summary_line_names_the_ledger( lambda **_kwargs: [{"repository": "leynos/statelet"}], ) - self._invoke( - [ - "--estate", - str(estate_path), - "--ledger", - str(ledger_path), - ] - ) + self._invoke([ + "--estate", + str(estate_path), + "--ledger", + str(ledger_path), + ]) captured = capsys.readouterr().out assert f"appended 1 record(s) to {ledger_path}" in captured, captured diff --git a/scripts/tests/test_parabellum_report.py b/scripts/tests/test_parabellum_report.py index d165b58..1f90109 100644 --- a/scripts/tests/test_parabellum_report.py +++ b/scripts/tests/test_parabellum_report.py @@ -30,6 +30,11 @@ def _record( introduce a key the ledger schema does not define. `audited_at` is a *required* field and gets its own parameter: the latest-record test varies it, which is not the same thing as supplying an optional detail. + + Returns + ------- + sweep.LedgerRecord + A complete ledger record for the test. """ finding: sweep.FindingRecord = { "rule_id": "QG-001", @@ -204,6 +209,11 @@ def _delimiter_count(row: str) -> int: A bare `row.count("|")` would count the escaping as a delimiter and so could not tell a broken row from a correctly escaped one. + + Returns + ------- + int + The number of unescaped cell delimiters in *row*. """ count = 0 escaped = False diff --git a/scripts/tests/test_typos_rollout.py b/scripts/tests/test_typos_rollout.py index cac7007..265729c 100644 --- a/scripts/tests/test_typos_rollout.py +++ b/scripts/tests/test_typos_rollout.py @@ -317,7 +317,7 @@ def __exit__(self, *_args: object) -> None: def open_response(request: urllib.request.Request, *, timeout: float) -> Response: """Capture the request passed to the network boundary.""" - assert timeout == 30.0 + assert timeout == pytest.approx(30.0) requests.append(request) return Response() diff --git a/scripts/typos_rollout.py b/scripts/typos_rollout.py index 24f77f8..49ba756 100644 --- a/scripts/typos_rollout.py +++ b/scripts/typos_rollout.py @@ -12,16 +12,14 @@ import urllib.parse import urllib.request -import typos_rollout_cache +from typos_rollout_cache import CacheTargets as _CacheTargets +from typos_rollout_cache import RefreshResult +from typos_rollout_cache import RemoteResponse as _RemoteResponse +from typos_rollout_cache import atomic_write as _atomic_write if typ.TYPE_CHECKING: import collections.abc as cabc -RefreshResult = typos_rollout_cache.RefreshResult -_CacheTargets = typos_rollout_cache.CacheTargets -_RemoteResponse = typos_rollout_cache.RemoteResponse -_atomic_write = typos_rollout_cache.atomic_write - SCHEMA_VERSION = 1 HTTP_NOT_MODIFIED = 304 SUFFIX_PAIRS = ( @@ -37,7 +35,7 @@ ) -@dc.dataclass(frozen=True) +@dc.dataclass(frozen=True, slots=True) class Dictionary: """Curated words and exclusions used to generate a ``typos`` config.""" @@ -114,12 +112,7 @@ def load_dictionary(path: pathlib.Path) -> Dictionary: def _merged_ignore_patterns(base: Dictionary, local: Dictionary) -> tuple[str, ...]: - """Union both ignore lists, then withdraw the overlay's removals. - - A removal that matches nothing is not an error: the shared base is - maintained elsewhere, so it may drop a pattern this repository had - already withdrawn, and that should not fail generation here. - """ + """Merge ignore patterns, applying removals and rejecting contradictions.""" removed = set(local.removed_patterns) if contradictory := removed & set(local.ignore_patterns): message = ( diff --git a/scripts/typos_rollout_cache.py b/scripts/typos_rollout_cache.py index fdc4b92..a01fbbb 100644 --- a/scripts/typos_rollout_cache.py +++ b/scripts/typos_rollout_cache.py @@ -11,7 +11,7 @@ import collections.abc as cabc -@dc.dataclass(frozen=True) +@dc.dataclass(frozen=True, slots=True) class RefreshResult: """Describe whether the untracked shared dictionary cache changed.""" @@ -19,7 +19,7 @@ class RefreshResult: cache: pathlib.Path -@dc.dataclass(frozen=True) +@dc.dataclass(frozen=True, slots=True) class CacheTargets: """Group the untracked dictionary cache and metadata sidecar paths.""" @@ -35,7 +35,7 @@ class RemoteResponse(typ.Protocol): def read(self) -> bytes: """Read the response body.""" - ... + pass def atomic_write(path: pathlib.Path, content: bytes) -> None: diff --git a/tests/bdd/test_estate_steps.py b/tests/bdd/test_estate_steps.py index d2d021b..a6ce927 100644 --- a/tests/bdd/test_estate_steps.py +++ b/tests/bdd/test_estate_steps.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import io import typing as typ from contextlib import redirect_stdout @@ -30,19 +31,13 @@ @pytest.fixture def betamax_recorder() -> typ.Iterator[typ.Callable[[str, requests.Session], None]]: """Provide a helper for starting betamax sessions.""" - contexts: list[typ.Any] = [] + with contextlib.ExitStack() as stack: - def start(name: str, session: requests.Session) -> None: - recorder = Betamax(session, cassette_library_dir=str(CASSETTE_DIR)) - ctx = recorder.use_cassette(name) - ctx.__enter__() - contexts.append(ctx) + def start(name: str, session: requests.Session) -> None: + recorder = Betamax(session, cassette_library_dir=str(CASSETTE_DIR)) + stack.enter_context(recorder.use_cassette(name)) - yield start - - while contexts: - ctx = contexts.pop() - ctx.__exit__(None, None, None) + yield start def _run_cli(arguments: list[str]) -> RunResult: @@ -60,10 +55,7 @@ def _run_cli(arguments: list[str]) -> RunResult: return RunResult( stdout=buffer.getvalue(), stderr="", returncode=int(exc.code or 0) ) - else: - return RunResult( - stdout=buffer.getvalue(), stderr="", returncode=int(result or 0) - ) + return RunResult(stdout=buffer.getvalue(), stderr="", returncode=int(result or 0)) @given("an empty concordat config directory", target_fixture="config_dir") @@ -296,17 +288,15 @@ def when_run_estate_init_local( cli_invocation: dict[str, RunResult], ) -> None: """Initialise an estate using the local remote.""" - cli_invocation["result"] = _run_cli( - [ - "estate", - "init", - alias, - str(local_remote_path), - "--github-owner", - owner, - "--yes", - ] - ) + cli_invocation["result"] = _run_cli([ + "estate", + "init", + alias, + str(local_remote_path), + "--github-owner", + owner, + "--yes", + ]) @then("the CLI prints") diff --git a/tests/bdd/test_execution_steps.py b/tests/bdd/test_execution_steps.py index 5610cc8..5b66496 100644 --- a/tests/bdd/test_execution_steps.py +++ b/tests/bdd/test_execution_steps.py @@ -301,12 +301,11 @@ def _run_cli(arguments: list[str]) -> RunResult: stderr=buffer_err.getvalue(), returncode=int(exc.code or 0), ) - else: - return RunResult( - stdout=buffer_out.getvalue(), - stderr=buffer_err.getvalue(), - returncode=int(result or 0), - ) + return RunResult( + stdout=buffer_out.getvalue(), + stderr=buffer_err.getvalue(), + returncode=int(result or 0), + ) @when(parsers.cfparse("I run concordat {command:w}")) diff --git a/tests/bdd/test_persist_steps.py b/tests/bdd/test_persist_steps.py index e27271c..a34fada 100644 --- a/tests/bdd/test_persist_steps.py +++ b/tests/bdd/test_persist_steps.py @@ -179,10 +179,9 @@ def _run_cli(arguments: list[str]) -> RunResult: return RunResult( stdout=buffer_out.getvalue(), stderr="", returncode=int(exc.code or 0) ) - else: - return RunResult( - stdout=buffer_out.getvalue(), stderr="", returncode=int(result or 0) - ) + return RunResult( + stdout=buffer_out.getvalue(), stderr="", returncode=int(result or 0) + ) def _seed_estate_remote(root: Path) -> Path: @@ -216,6 +215,11 @@ def given_config_dir(config_dir: Path) -> Path: Shared by active-estate and explicit --alias persistence scenarios so each scenario interacts with its own config namespace. + + Returns + ------- + Path + The isolated configuration directory. """ return config_dir diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 88c6e46..16dacb7 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -57,9 +57,14 @@ def mock_bootstrap(mocker: pytest_mock.MockFixture) -> mock.Mock: def fake_github_client(mocker: pytest_mock.MockFixture) -> mock.Mock: """Patch `_build_client` with a GitHub client that has no repository yet. - The organisation mock is reachable as + The organization mock is reachable as ``fake_github_client.organization.return_value`` for tests that assert the repository was created through it. + + Returns + ------- + mock.Mock + The patched GitHub client mock. """ client = mocker.Mock() client.repository.return_value = None @@ -77,6 +82,11 @@ def xdg_env( Shared so no test reads or writes the developer's real XDG directories, and so an active owner written by one test cannot be observed by another. + + Returns + ------- + dict[str, str] + The XDG environment mapping installed for the test. """ mapping = { "XDG_CONFIG_HOME": str(tmp_path / "config"), @@ -160,15 +170,13 @@ def persist_repo_setup( @pytest.fixture def persist_prompts() -> typ.Iterator[str]: """Return standard prompt responses for persistence flows.""" - return iter( - [ - "df12", - "fr-par", - "https://s3.fr-par.scw.cloud", - "estates/example/main", - "terraform.tfstate", - ] - ) + return iter([ + "df12", + "fr-par", + "https://s3.fr-par.scw.cloud", + "estates/example/main", + "terraform.tfstate", + ]) @pytest.fixture @@ -224,6 +232,11 @@ def persist_test_context( XDG isolation arrives through `persist_monkeypatch_base`, which owns the shared environment setup these tests need. + + Returns + ------- + PersistTestContext + The grouped fixtures used by persist-estate tests. """ workdir, repo, bare, record = persist_repo_setup return PersistTestContext( diff --git a/tests/unit/test_canon_artifacts.py b/tests/unit/test_canon_artifacts.py index ccad728..1120aab 100644 --- a/tests/unit/test_canon_artifacts.py +++ b/tests/unit/test_canon_artifacts.py @@ -43,18 +43,16 @@ def _write_manifest_via_yaml( ) -> None: manifest_path.parent.mkdir(parents=True, exist_ok=True) manifest_path.write_text( - "\n".join( - [ - "schema_version: 1", - "artifacts:", - f" - id: {artifact_id}", - " type: lint-config", - f" path: {artifact_path}", - " description: test artifact", - f" sha256: {sha256}", - "", - ] - ), + "\n".join([ + "schema_version: 1", + "artifacts:", + f" - id: {artifact_id}", + " type: lint-config", + f" path: {artifact_path}", + " description: test artifact", + f" sha256: {sha256}", + "", + ]), encoding="utf-8", ) @@ -67,15 +65,13 @@ def _write_manifest_entries( manifest_path.parent.mkdir(parents=True, exist_ok=True) lines: list[str] = ["schema_version: 1", "artifacts:"] for artifact in artifacts: - lines.extend( - [ - f" - id: {artifact['id']}", - f" type: {artifact['type']}", - f" path: {artifact['path']}", - f" description: {artifact['description']}", - f" sha256: {artifact['sha256']}", - ] - ) + lines.extend([ + f" - id: {artifact['id']}", + f" type: {artifact['type']}", + f" path: {artifact['path']}", + f" description: {artifact['description']}", + f" sha256: {artifact['sha256']}", + ]) lines.append("") manifest_path.write_text("\n".join(lines), encoding="utf-8") @@ -101,15 +97,13 @@ def _setup_multi_artifact_scenario( for artifact_id, relative_path, template_content, published_content in artifacts: _write_template_file(root, relative_path, template_content) sha = hashlib.sha256(template_content.encode("utf-8")).hexdigest() - entries.append( - { - "id": artifact_id, - "type": "lint-config", - "path": relative_path, - "description": artifact_id, - "sha256": sha, - } - ) + entries.append({ + "id": artifact_id, + "type": "lint-config", + "path": relative_path, + "description": artifact_id, + "sha256": sha, + }) if published_content is None: continue @@ -483,14 +477,12 @@ def test_load_manifest_rejects_non_mapping_artifact_entry(tmp_path: Path) -> Non """load_manifest rejects artifacts that are not mappings.""" manifest_path = tmp_path / "manifest.yaml" manifest_path.write_text( - "\n".join( - [ - "schema_version: 1", - "artifacts:", - " - 1", - "", - ] - ), + "\n".join([ + "schema_version: 1", + "artifacts:", + " - 1", + "", + ]), encoding="utf-8", ) @@ -505,17 +497,15 @@ def test_load_manifest_rejects_missing_artifact_key(tmp_path: Path) -> None: """load_manifest rejects artifact entries missing required keys.""" manifest_path = tmp_path / "manifest.yaml" manifest_path.write_text( - "\n".join( - [ - "schema_version: 1", - "artifacts:", - " - id: missing-key", - " type: lint-config", - " path: platform-standards/canon/lint/python/ruff.toml", - " description: test artifact", - "", - ] - ), + "\n".join([ + "schema_version: 1", + "artifacts:", + " - id: missing-key", + " type: lint-config", + " path: platform-standards/canon/lint/python/ruff.toml", + " description: test artifact", + "", + ]), encoding="utf-8", ) diff --git a/tests/unit/test_canon_artifacts_cli.py b/tests/unit/test_canon_artifacts_cli.py index fe81025..9b8640f 100644 --- a/tests/unit/test_canon_artifacts_cli.py +++ b/tests/unit/test_canon_artifacts_cli.py @@ -35,18 +35,16 @@ def _write_template_and_manifest( manifest_path = template_root / "platform-standards" / "canon" / "manifest.yaml" manifest_path.parent.mkdir(parents=True, exist_ok=True) manifest_path.write_text( - "\n".join( - [ - "schema_version: 1", - "artifacts:", - " - id: python-ruff-config", - " type: lint-config", - " path: platform-standards/canon/lint/python/ruff.toml", - " description: test artifact", - f" sha256: {sha}", - "", - ] - ), + "\n".join([ + "schema_version: 1", + "artifacts:", + " - id: python-ruff-config", + " type: lint-config", + " path: platform-standards/canon/lint/python/ruff.toml", + " description: test artifact", + f" sha256: {sha}", + "", + ]), encoding="utf-8", ) @@ -59,15 +57,13 @@ def _write_manifest_entries( manifest_path.parent.mkdir(parents=True, exist_ok=True) lines: list[str] = ["schema_version: 1", "artifacts:"] for entry in entries: - lines.extend( - [ - f" - id: {entry['id']}", - f" type: {entry['type']}", - f" path: {entry['path']}", - f" description: {entry['description']}", - f" sha256: {entry['sha256']}", - ] - ) + lines.extend([ + f" - id: {entry['id']}", + f" type: {entry['type']}", + f" path: {entry['path']}", + f" description: {entry['description']}", + f" sha256: {entry['sha256']}", + ]) lines.append("") manifest_path.write_text("\n".join(lines), encoding="utf-8") @@ -99,15 +95,13 @@ def _setup_multi_artifact_cli_scenario( template_root, relative_path, template_content ) sha = hashlib.sha256(template_file.read_bytes()).hexdigest() - entries.append( - { - "id": artifact_id, - "type": artifact_type, - "path": relative_path, - "description": artifact_id, - "sha256": sha, - } - ) + entries.append({ + "id": artifact_id, + "type": artifact_type, + "path": relative_path, + "description": artifact_id, + "sha256": sha, + }) if published_content is None: continue diff --git a/tests/unit/test_canon_artifacts_tui.py b/tests/unit/test_canon_artifacts_tui.py new file mode 100644 index 0000000..db10f39 --- /dev/null +++ b/tests/unit/test_canon_artifacts_tui.py @@ -0,0 +1,72 @@ +"""Behavioural tests for the canonical-artifacts Textual application.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from textual.widgets import DataTable + +from concordat.canon_artifacts import ( + ArtifactComparison, + ArtifactStatus, + CanonArtifact, + CanonManifest, +) +from scripts import canon_artifacts_tui + + +def _comparison(identifier: str) -> ArtifactComparison: + """Build a comparison displayed by the Textual application.""" + artifact = CanonArtifact( + id=identifier, + type="lint-config", + path=Path("canon/lint/python/ruff.toml"), + description="Ruff configuration", + sha256="a" * 64, + ) + return ArtifactComparison( + artifact=artifact, + template_sha256="b" * 64, + published_sha256=None, + status=ArtifactStatus.MISSING, + published_path=Path("canon/lint/python/ruff.toml"), + ) + + +@pytest.mark.asyncio +async def test_refresh_action_recomputes_mounted_application_rows( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + """The public refresh binding replaces the mounted table's comparisons.""" + initial = [_comparison("initial")] + refreshed = [_comparison("first"), _comparison("second")] + comparison_sets = iter((initial, refreshed)) + monkeypatch.setattr( + canon_artifacts_tui, + "compare_manifest_to_published", + lambda *_, **__: next(comparison_sets), + ) + app = canon_artifacts_tui.CanonArtifactsApp( + manifest=CanonManifest( + schema_version=1, + artifacts=(), + manifest_path=tmp_path / "canon" / "manifest.yaml", + ), + published_root=tmp_path, + ids=None, + types=None, + ) + + async with app.run_test() as pilot: + table = app.query_one(DataTable) + assert table.row_count == 1, "refresh test requires one initial table row" + + await pilot.press("r") + + assert table.row_count == 2, "refresh should replace the table with two rows" + assert [table.get_row_at(index)[0] for index in range(table.row_count)] == [ + "first", + "second", + ], "refresh should display the recomputed comparison IDs" diff --git a/tests/unit/test_dataclass_slots.py b/tests/unit/test_dataclass_slots.py new file mode 100644 index 0000000..6aaa748 --- /dev/null +++ b/tests/unit/test_dataclass_slots.py @@ -0,0 +1,284 @@ +"""Contracts for production dataclasses whose storage is explicitly slotted.""" + +from __future__ import annotations + +import typing as typ + +import pytest + +from concordat import enrol, estate_execution, platform_standards +from concordat.auditor import models as auditor_models +from concordat.auditor import priority +from concordat.persistence import models as persistence_models + + +@pytest.mark.parametrize( + ("model_type", "expected_fields"), + [ + pytest.param( + auditor_models.RepositorySnapshot, + ( + "owner", + "name", + "default_branch", + "allow_squash_merge", + "allow_merge_commit", + "allow_rebase_merge", + "allow_auto_merge", + "delete_branch_on_merge", + ), + id="repository-snapshot", + ), + pytest.param( + auditor_models.RequiredStatusChecks, + ("strict", "contexts"), + id="required-status-checks", + ), + pytest.param( + auditor_models.RequiredPullRequestReviews, + ( + "required_approvals", + "dismiss_stale_reviews", + "require_code_owner_reviews", + ), + id="required-pull-request-reviews", + ), + pytest.param( + auditor_models.BranchProtection, + ( + "enforce_admins", + "require_signed_commits", + "required_linear_history", + "require_conversation_resolution", + "allows_deletions", + "allows_force_pushes", + "status_checks", + "pull_request_reviews", + ), + id="branch-protection", + ), + pytest.param( + auditor_models.TeamPermission, + ("slug", "permission"), + id="team-permission", + ), + pytest.param( + auditor_models.CollaboratorPermission, + ("login", "permission", "permissions"), + id="collaborator-permission", + ), + pytest.param( + auditor_models.LabelState, + ("name", "color", "description"), + id="label-state", + ), + pytest.param( + auditor_models.AuditContext, + ( + "repository", + "branch_protection", + "teams", + "collaborators", + "labels", + "priority_model", + ), + id="audit-context", + ), + pytest.param( + auditor_models.CheckDefinition, + ( + "rule_id", + "name", + "short_description", + "long_description", + "level", + "help_uri", + ), + id="check-definition", + ), + pytest.param( + auditor_models.Finding, + ("rule_id", "message", "level", "resource", "properties"), + id="finding", + ), + pytest.param( + priority.PriorityLabel, + ("key", "name", "color", "description"), + id="priority-label", + ), + pytest.param( + priority.PriorityFieldOption, + ("key", "display_name"), + id="priority-field-option", + ), + pytest.param( + priority.PriorityField, + ("name", "type", "options"), + id="priority-field", + ), + pytest.param( + priority.PriorityModel, + ("schema_version", "labels", "field", "aliases"), + id="priority-model", + ), + pytest.param( + enrol.EnrollmentOutcome, + ("repository", "location", "created", "committed", "pushed", "platform_pr"), + id="enrollment-outcome", + ), + pytest.param( + enrol.DisenrollmentOutcome, + ( + "repository", + "location", + "updated", + "missing_document", + "committed", + "pushed", + "platform_pr", + ), + id="disenrollment-outcome", + ), + pytest.param( + enrol._RepositoryContext, + ("repository", "location", "is_remote", "callbacks"), + id="repository-context", + ), + pytest.param( + estate_execution.ExecutionOptions, + ( + "github_owner", + "github_token", + "extra_args", + "keep_workdir", + "cache_directory", + "environment", + ), + id="execution-options", + ), + pytest.param( + estate_execution.ExecutionIO, + ("stdout", "stderr"), + id="execution-io", + ), + pytest.param( + estate_execution.WorkspaceContext, + ("root", "tofu_dir"), + id="execution-workspace-context", + ), + pytest.param( + estate_execution.ExecutionContext, + ("options", "io", "env"), + id="execution-context", + ), + pytest.param( + estate_execution.PersistenceRuntime, + ("descriptor", "backend_config", "object_key", "env_overrides"), + id="execution-persistence-runtime", + ), + pytest.param( + persistence_models.PersistenceDescriptor, + ( + "schema_version", + "enabled", + "bucket", + "key_prefix", + "key_suffix", + "region", + "endpoint", + "backend_config_path", + "notification_topic", + ), + id="persistence-descriptor", + ), + pytest.param( + persistence_models.PersistenceResult, + ("backend_path", "manifest_path", "branch", "pr_url", "updated", "message"), + id="persistence-result", + ), + pytest.param( + persistence_models.PersistenceFiles, + ("backend_path", "backend_contents", "manifest_path", "manifest_contents"), + id="persistence-files", + ), + pytest.param( + persistence_models.PersistenceOptions, + ( + "force", + "github_token", + "input_func", + "s3_client_factory", + "pr_opener", + "fmt_runner", + "timestamp_factory", + "allow_insecure_endpoint", + "bucket", + "region", + "endpoint", + "key_prefix", + "key_suffix", + "no_input", + ), + id="persistence-options", + ), + pytest.param( + persistence_models.PullRequestContext, + ( + "record", + "branch_name", + "descriptor", + "key_suffix", + "github_token", + "pr_opener", + ), + id="pull-request-context", + ), + pytest.param( + persistence_models.PersistencePaths, + ("manifest_path", "backend_path"), + id="persistence-paths", + ), + pytest.param( + persistence_models.WorkspaceContext, + ("workdir", "repository"), + id="persistence-workspace-context", + ), + pytest.param( + persistence_models.FinalizationContext, + ( + "record", + "branch_name", + "descriptor", + "key_suffix", + "github_token", + "opts", + ), + id="finalization-context", + ), + pytest.param( + platform_standards.PlatformStandardsConfig, + ("repo_url", "base_branch", "inventory_path", "github_token"), + id="platform-standards-config", + ), + pytest.param( + platform_standards.PlatformStandardsResult, + ("created", "branch", "pr_url", "message"), + id="platform-standards-result", + ), + ], +) +def test_recent_dataclasses_store_their_declared_fields_in_slots( + model_type: type[object], + expected_fields: tuple[str, ...], +) -> None: + """Keep each changed dataclass's declared fields in slotted storage.""" + actual_fields = tuple( + typ.cast("dict[str, object]", model_type.__dict__["__dataclass_fields__"]) + ) + slots = typ.cast("tuple[str, ...]", model_type.__dict__["__slots__"]) + instance = object.__new__(model_type) + + assert actual_fields == expected_fields + assert slots == expected_fields + with pytest.raises(AttributeError): + object.__setattr__(instance, "undeclared_field", None) diff --git a/tests/unit/test_estate_github.py b/tests/unit/test_estate_github.py index f07f6f4..8ee4abe 100644 --- a/tests/unit/test_estate_github.py +++ b/tests/unit/test_estate_github.py @@ -1,6 +1,6 @@ """Unit tests for the GitHub API helpers in `concordat.estate_github`. -Client construction and the organisation/personal repository-creation paths are +Client construction and the organization/personal repository-creation paths are exercised directly; the provisioning flow that drives them is covered by the `init_estate` suite. """ @@ -29,33 +29,33 @@ type Rejection = type[github3_exceptions.ResponseError] -def _client_rejecting_organisation_lookup( +def _client_rejecting_organization_lookup( mocker: pytest_mock.MockFixture, rejection: Rejection, ) -> github3.GitHub: - """Return a client whose organisation lookup is rejected.""" + """Return a client whose organization lookup is rejected.""" client = mocker.Mock() client.organization.side_effect = rejection(mocker.Mock()) return client -def _look_up_organisation(client: github3.GitHub) -> None: - """Resolve the organisation that would own the estate repository.""" +def _look_up_organization(client: github3.GitHub) -> None: + """Resolve the organization that would own the estate repository.""" estate_github._find_organization(client, "example") -def _organisation_rejecting_creation( +def _organization_rejecting_creation( mocker: pytest_mock.MockFixture, rejection: Rejection, ) -> github3.orgs.Organization: - """Return an organisation that refuses to create a repository.""" + """Return an organization that refuses to create a repository.""" org = mocker.Mock() org.create_repository.side_effect = rejection(mocker.Mock()) return org -def _create_organisation_repository(org: github3.orgs.Organization) -> None: - """Create the estate repository inside an organisation.""" +def _create_organization_repository(org: github3.orgs.Organization) -> None: + """Create the estate repository inside an organization.""" estate_github._create_organization_repository(org, "example", "core") @@ -67,6 +67,11 @@ def _client_rejecting_personal_creation( ``me()`` must still identify the expected owner, since the permission check precedes the creation call this scenario is about. + + Returns + ------- + github3.GitHub + An authenticated client configured to reject repository creation. """ client = mocker.Mock() client.me.return_value = mocker.Mock(login="example") @@ -93,7 +98,7 @@ class AuthenticationFailureScenario: class TestCreateRepository: - """Contracts of the organisation/personal repository-creation helpers. + """Contracts of the organization/personal repository-creation helpers. The helpers live in ``estate_github``; ``_create_repository`` remains the orchestration seam that ``estate_repository`` re-exports and tests patch. @@ -120,19 +125,19 @@ class TestCreateRepository: [ pytest.param( AuthenticationFailureScenario( - setup=_client_rejecting_organisation_lookup, - invoke=_look_up_organisation, + setup=_client_rejecting_organization_lookup, + invoke=_look_up_organization, error_type=estate.GitHubOrganizationAuthenticationError, ), - id="organisation-lookup", + id="organization-lookup", ), pytest.param( AuthenticationFailureScenario( - setup=_organisation_rejecting_creation, - invoke=_create_organisation_repository, + setup=_organization_rejecting_creation, + invoke=_create_organization_repository, error_type=estate.GitHubRepositoryCreationAuthenticationError, ), - id="organisation-creation", + id="organization-creation", ), pytest.param( AuthenticationFailureScenario( @@ -153,7 +158,7 @@ def test_rejected_call_is_translated( """Each rejected call reports the error naming its own boundary. The boundaries share a rejection but not a diagnosis: a refused lookup - says nothing about whether the owner is an organisation, so each keeps + says nothing about whether the owner is an organization, so each keeps a distinct error class. Both a 401 and a 403 must translate, since github3 models them as unrelated siblings. """ @@ -162,25 +167,25 @@ def test_rejected_call_is_translated( with pytest.raises(scenario.error_type): scenario.invoke(subject) - def test_missing_organisation_returns_none( + def test_missing_organization_returns_none( self, mocker: pytest_mock.MockFixture, ) -> None: - """A NotFound organisation selects the personal-owner path.""" + """A NotFound organization selects the personal-owner path.""" client = mocker.Mock() client.organization.side_effect = github3_exceptions.NotFoundError( mocker.Mock() ) assert estate_github._find_organization(client, "example") is None, ( - "a missing organisation should resolve to None" + "a missing organization should resolve to None" ) - def test_organisation_path_never_touches_personal_methods( + def test_organization_path_never_touches_personal_methods( self, mocker: pytest_mock.MockFixture, ) -> None: - """An existing organisation short-circuits the personal-owner path.""" + """An existing organization short-circuits the personal-owner path.""" client = mocker.Mock() org = mocker.Mock() client.organization.return_value = org @@ -219,7 +224,7 @@ def test_personal_path_creates_with_unchanged_options( self, mocker: pytest_mock.MockFixture, ) -> None: - """A missing organisation creates the repository for its owner.""" + """A missing organization creates the repository for its owner.""" client = mocker.Mock() client.organization.side_effect = github3_exceptions.NotFoundError( mocker.Mock() diff --git a/tests/unit/test_persistence_s3_credentials.py b/tests/unit/test_persistence_s3_credentials.py index bf114df..eeb9015 100644 --- a/tests/unit/test_persistence_s3_credentials.py +++ b/tests/unit/test_persistence_s3_credentials.py @@ -65,6 +65,11 @@ def captured_client_kwargs( owner tree to this test's `tmp_path` for the same reason — the factory falls back to the active owner's credentials file when no `owner=` is given, and that lookup must not find one written by another test. + + Returns + ------- + CapturedCall + The captured boto3 service name and keyword arguments. """ del xdg_env for variable in ( diff --git a/tests/unit/test_platform_standards_inventory.py b/tests/unit/test_platform_standards_inventory.py index 3a788a3..01aced8 100644 --- a/tests/unit/test_platform_standards_inventory.py +++ b/tests/unit/test_platform_standards_inventory.py @@ -3,15 +3,17 @@ from __future__ import annotations import typing as typ +from pathlib import Path +from unittest import mock +import pygit2 import pytest +from hypothesis import given +from hypothesis import strategies as st from ruamel.yaml import YAML from concordat import platform_standards -if typ.TYPE_CHECKING: - from pathlib import Path - def _seed_inventory_with_metadata(inventory: Path, repos: list[str]) -> None: """Write an inventory file with schema_version, metadata, labels, and repos.""" @@ -43,6 +45,181 @@ def _assert_metadata_preserved(data: dict[str, typ.Any]) -> None: assert data["labels"] == ["backend", "critical"] +@pytest.mark.parametrize( + ("mutation_result", "expected_changed", "expected_calls"), + [ + pytest.param(False, False, ["mutate"], id="unchanged"), + pytest.param(True, True, ["mutate", "commit", "validate"], id="changed"), + ], +) +def test_apply_inventory_change_commits_and_validates_only_when_mutated( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + *, + mutation_result: bool, + expected_changed: bool, + expected_calls: list[str], +) -> None: + """Only commit and validate after an inventory mutation.""" + calls: list[str] = [] + config = platform_standards.PlatformStandardsConfig( + repo_url="https://example.com/platform-standards.git" + ) + + def mutate_inventory(inventory: Path, repo_slug: str) -> bool: + calls.append("mutate") + assert inventory == tmp_path / config.inventory_path, ( + "mutator should receive the configured inventory path" + ) + assert repo_slug == "example/repo", "mutator should receive the repository slug" + return mutation_result + + def commit_inventory_changes(*args: object, **kwargs: object) -> None: + calls.append("commit") + + def validate_tofu_changes(workdir: Path) -> None: + assert workdir == tmp_path, "validation should run in the repository worktree" + calls.append("validate") + + monkeypatch.setattr( + platform_standards, + "_commit_inventory_changes", + commit_inventory_changes, + ) + monkeypatch.setattr( + platform_standards, + "_validate_tofu_changes", + validate_tofu_changes, + ) + + changed = platform_standards._apply_inventory_change( + typ.cast("pygit2.Repository", object()), + tmp_path, + config, + "example/repo", + typ.cast("pygit2.Commit", object()), + verb="enrol", + mutate_inventory=mutate_inventory, + ) + + assert changed is expected_changed + assert calls == expected_calls + + +@given(mutation_results=st.lists(st.booleans(), min_size=1, max_size=20)) +def test_apply_inventory_change_follows_the_mutation_trace( + mutation_results: list[bool], +) -> None: + """Commit and validate exactly after a changed mutation.""" + calls: list[str] = [] + mutations = iter(mutation_results) + config = platform_standards.PlatformStandardsConfig( + repo_url="https://example.com/platform-standards.git" + ) + + def mutate_inventory(inventory: Path, repo_slug: str) -> bool: + calls.append("mutate") + assert inventory == Path("workspace") / config.inventory_path + assert repo_slug == "example/repo" + return next(mutations) + + def commit_inventory_changes(*args: object, **kwargs: object) -> None: + calls.append("commit") + + def validate_tofu_changes(workdir: Path) -> None: + assert workdir == Path("workspace") + calls.append("validate") + + with ( + mock.patch.object( + platform_standards, + "_commit_inventory_changes", + commit_inventory_changes, + ), + mock.patch.object( + platform_standards, + "_validate_tofu_changes", + validate_tofu_changes, + ), + ): + changed = [ + platform_standards._apply_inventory_change( + typ.cast("pygit2.Repository", object()), + Path("workspace"), + config, + "example/repo", + typ.cast("pygit2.Commit", object()), + verb="enrol", + mutate_inventory=mutate_inventory, + ) + for _ in mutation_results + ] + + expected_calls = [ + call + for mutation_result in mutation_results + for call in ( + ("mutate", "commit", "validate") if mutation_result else ("mutate",) + ) + ] + assert changed == mutation_results + assert calls == expected_calls + + +def test_apply_inventory_change_commits_the_mutated_inventory( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + """A changed inventory is committed before its validation boundary runs.""" + repository = pygit2.init_repository(str(tmp_path)) + repository.config["user.name"] = "Test User" + repository.config["user.email"] = "test@example.com" + config = platform_standards.PlatformStandardsConfig( + repo_url="https://example.com/platform-standards.git" + ) + inventory_path = tmp_path / config.inventory_path + inventory_path.parent.mkdir(parents=True) + inventory_path.write_text("schema_version: 1\nrepositories: []\n", encoding="utf-8") + repository.index.add(config.inventory_path) + repository.index.write() + signature = repository.default_signature + base_commit_id = repository.create_commit( + "HEAD", + signature, + signature, + "seed inventory", + repository.index.write_tree(), + [], + ) + base_commit = repository[base_commit_id].peel(pygit2.Commit) + validated_heads: list[pygit2.Oid] = [] + + def validate_tofu_changes(workdir: Path) -> None: + assert workdir == tmp_path, "validation should run in the repository worktree" + validated_heads.append(repository.head.peel(pygit2.Commit).id) + + monkeypatch.setattr( + platform_standards, "_validate_tofu_changes", validate_tofu_changes + ) + + changed = platform_standards._apply_inventory_change( + repository, + tmp_path, + config, + "example/repo", + base_commit, + verb="enrol", + mutate_inventory=platform_standards._update_inventory, + ) + + commit = repository.head.peel(pygit2.Commit) + assert changed is True + assert commit.parent_ids == [base_commit.id] + assert commit.message == "chore: enrol example/repo via concordat" + assert "example/repo" in inventory_path.read_text(encoding="utf-8") + assert validated_heads == [commit.id] + + def test_update_inventory_adds_entry(tmp_path: Path) -> None: """Add a repository when it is not present.""" inventory = tmp_path / "repositories.yaml" diff --git a/tests/unit/test_platform_standards_pr_push.py b/tests/unit/test_platform_standards_pr_push.py index ee220dc..6240786 100644 --- a/tests/unit/test_platform_standards_pr_push.py +++ b/tests/unit/test_platform_standards_pr_push.py @@ -63,13 +63,11 @@ def platform_origin(tmp_path: pathlib.Path) -> tuple[pathlib.Path, str]: seed_repo.config["user.name"] = "Test User" seed_repo.config["user.email"] = "test@example.com" - inventory = "\n".join( - [ - "schema_version: 1", - "repositories: []", - "", - ] - ) + inventory = "\n".join([ + "schema_version: 1", + "repositories: []", + "", + ]) _commit_file( seed_repo, relative_path="tofu/inventory/repositories.yaml", @@ -181,14 +179,12 @@ def test_ensure_repository_pr_reports_existing_branch_pr_when_not_merged( local_branch = work_repo.create_branch(branch_name, base) work_repo.checkout(local_branch) - inventory_with_repo = "\n".join( - [ - "schema_version: 1", - "repositories:", - " - name: test-owner/test-repo", - "", - ] - ) + inventory_with_repo = "\n".join([ + "schema_version: 1", + "repositories:", + " - name: test-owner/test-repo", + "", + ]) _commit_file( work_repo, relative_path="tofu/inventory/repositories.yaml", diff --git a/tests/unit/test_properties.py b/tests/unit/test_properties.py index fa1340b..491fbf4 100644 --- a/tests/unit/test_properties.py +++ b/tests/unit/test_properties.py @@ -31,6 +31,11 @@ def _is_valid_owner(owner: str) -> bool: Written from the rule — alphanumeric ends, alphanumerics and hyphens within — rather than from `xdg._OWNER_PATTERN`, so a change to the pattern is a change this can detect. + + Returns + ------- + bool + Whether *owner* satisfies the grammar. """ if not owner: return False @@ -204,6 +209,11 @@ def _record(repository: str, verdict: str, sha: str | None) -> sweep.LedgerRecor Built through `_base_record` rather than by hand, so the generated histories carry every required key and stay valid `LedgerRecord` values. + + Returns + ------- + sweep.LedgerRecord + A complete ledger record with the requested fields varied. """ record = sweep._base_record(repository) record["verdict"] = verdict diff --git a/tests/unit/test_rule_rendering_cli.py b/tests/unit/test_rule_rendering_cli.py index 4a6eafe..f84a154 100644 --- a/tests/unit/test_rule_rendering_cli.py +++ b/tests/unit/test_rule_rendering_cli.py @@ -75,30 +75,26 @@ def test_json_format_emits_structured_document( _write_checkout(tmp_path, cargo=True, makefile=True) cmd_mox.mock("makeutil").returns(stdout=json.dumps(MINIMAL_REPORT)) - clean_doc = json.dumps( - [ - { - "filename": "envelope.json", - "namespace": "canon.lint_rules.rust_makefile_baseline", - "successes": 14, - } - ] - ) + clean_doc = json.dumps([ + { + "filename": "envelope.json", + "namespace": "canon.lint_rules.rust_makefile_baseline", + "successes": 14, + } + ]) cmd_mox.mock("conftest").returns(exit_code=0, stdout=clean_doc) cmd_mox.replay() try: - exit_code = cli.main( - [ - "artefact", - "rule", - "run", - "rust-makefile-baseline", - "--repo", - str(tmp_path), - "--format", - "json", - ] - ) + exit_code = cli.main([ + "artefact", + "rule", + "run", + "rust-makefile-baseline", + "--repo", + str(tmp_path), + "--format", + "json", + ]) except SystemExit as exc: exit_code = int(exc.code or 0) cmd_mox.verify() diff --git a/tests/unit/test_run_apply_auto_state_rm.py b/tests/unit/test_run_apply_auto_state_rm.py index 00c1272..b355d8a 100644 --- a/tests/unit/test_run_apply_auto_state_rm.py +++ b/tests/unit/test_run_apply_auto_state_rm.py @@ -156,7 +156,9 @@ def _setup_test_environment( ) -> tuple[list[list[str]], TofuMockBuilder, ExecutionIO, ExecutionOptions]: """Set up common test environment for run_apply tests. - Returns: + Returns + ------- + tuple[list[list[str]], TofuMockBuilder, ExecutionIO, ExecutionOptions] Tuple of (calls list, mock_builder, io_streams, options). """ @@ -209,7 +211,8 @@ def test_run_apply_offers_to_forget_resources_on_prevent_destroy( ) tofu_mock = ( - builder.with_apply_response(stderr=_PREVENT_DESTROY_ERROR, returncode=1) + builder + .with_apply_response(stderr=_PREVENT_DESTROY_ERROR, returncode=1) .with_apply_response(returncode=0) .with_state_list_response(stdout=_STATE_LIST_OUTPUT) .with_state_rm_response(returncode=0) @@ -289,7 +292,8 @@ def test_run_apply_prevent_destroy_state_list_no_matches( ) tofu_mock = ( - builder.with_apply_response(stderr=_PREVENT_DESTROY_ERROR, returncode=1) + builder + .with_apply_response(stderr=_PREVENT_DESTROY_ERROR, returncode=1) .with_state_list_response(stdout="") # Empty state list .build(calls) ) @@ -314,7 +318,8 @@ def test_run_apply_prevent_destroy_state_rm_failure( ) tofu_mock = ( - builder.with_apply_response(stderr=_PREVENT_DESTROY_ERROR, returncode=1) + builder + .with_apply_response(stderr=_PREVENT_DESTROY_ERROR, returncode=1) .with_state_list_response(stdout=_STATE_LIST_OUTPUT) .with_state_rm_response( stderr="Error: failed to remove state entry", returncode=1 @@ -342,7 +347,8 @@ def test_run_apply_state_list_returns_nonzero( ) tofu_mock = ( - builder.with_apply_response(stderr=_PREVENT_DESTROY_ERROR, returncode=1) + builder + .with_apply_response(stderr=_PREVENT_DESTROY_ERROR, returncode=1) .with_state_list_response(stderr="Error: failed to list state", returncode=1) .build(calls) ) diff --git a/tests/unit/test_run_plan.py b/tests/unit/test_run_plan.py index b650a35..2ead3cb 100644 --- a/tests/unit/test_run_plan.py +++ b/tests/unit/test_run_plan.py @@ -210,16 +210,14 @@ def test_run_plan_sanitizes_inventory_yaml_directives_for_tofu( inventory_path = tofu_root / "inventory" / "repositories.yaml" inventory_path.parent.mkdir(parents=True, exist_ok=True) inventory_path.write_text( - "\n".join( - [ - "%YAML 1.2", - "---", - "schema_version: 1", - "repositories: []", - "...", - "", - ] - ), + "\n".join([ + "%YAML 1.2", + "---", + "schema_version: 1", + "repositories: []", + "...", + "", + ]), encoding="utf-8", ) diff --git a/tests/unit/test_runner.py b/tests/unit/test_runner.py index d62a75d..2f37f66 100644 --- a/tests/unit/test_runner.py +++ b/tests/unit/test_runner.py @@ -98,37 +98,35 @@ def test_failures_map_to_findings( """Failures map to findings.""" _write_checkout(tmp_path, cargo=True, makefile=True) cmd_mox.mock("makeutil").returns(stdout=json.dumps(MINIMAL_REPORT)) - conftest_doc = json.dumps( - [ - { - "filename": "envelope.json", - "namespace": "canon.lint_rules.rust_makefile_baseline", - "successes": 12, - "failures": [ - { - "msg": 'required Make target "lint" is absent', - "metadata": { - "line": 0, - "path": "Makefile", - "rule_id": "FP-003", - "severity": "error", - "verdict": "noncompliant", - }, + conftest_doc = json.dumps([ + { + "filename": "envelope.json", + "namespace": "canon.lint_rules.rust_makefile_baseline", + "successes": 12, + "failures": [ + { + "msg": 'required Make target "lint" is absent', + "metadata": { + "line": 0, + "path": "Makefile", + "rule_id": "FP-003", + "severity": "error", + "verdict": "noncompliant", }, - { - "msg": "cannot prove the gate", - "metadata": { - "line": 3, - "path": "Makefile", - "rule_id": "QG-001", - "severity": "error", - "verdict": "indeterminate", - }, + }, + { + "msg": "cannot prove the gate", + "metadata": { + "line": 3, + "path": "Makefile", + "rule_id": "QG-001", + "severity": "error", + "verdict": "indeterminate", }, - ], - } - ] - ) + }, + ], + } + ]) cmd_mox.mock("conftest").returns(exit_code=1, stdout=conftest_doc) cmd_mox.replay() result = run_rule("rust-makefile-baseline", tmp_path) @@ -148,15 +146,13 @@ def test_clean_run_is_compliant( """Clean run is compliant.""" _write_checkout(tmp_path, cargo=True, makefile=True) cmd_mox.mock("makeutil").returns(stdout=json.dumps(MINIMAL_REPORT)) - clean_doc = json.dumps( - [ - { - "filename": "envelope.json", - "namespace": "canon.lint_rules.rust_makefile_baseline", - "successes": 14, - } - ] - ) + clean_doc = json.dumps([ + { + "filename": "envelope.json", + "namespace": "canon.lint_rules.rust_makefile_baseline", + "successes": 14, + } + ]) cmd_mox.mock("conftest").returns(exit_code=0, stdout=clean_doc) cmd_mox.replay() result = run_rule("rust-makefile-baseline", tmp_path) @@ -172,27 +168,25 @@ def test_indeterminate_only_yields_indeterminate_verdict( """Indeterminate only yields indeterminate verdict.""" _write_checkout(tmp_path, cargo=True, makefile=True) cmd_mox.mock("makeutil").returns(stdout=json.dumps(MINIMAL_REPORT)) - doc = json.dumps( - [ - { - "filename": "envelope.json", - "namespace": "canon.lint_rules.rust_makefile_baseline", - "successes": 13, - "failures": [ - { - "msg": "cannot prove the gate", - "metadata": { - "line": 0, - "path": "Makefile", - "rule_id": "QG-001", - "severity": "error", - "verdict": "indeterminate", - }, - } - ], - } - ] - ) + doc = json.dumps([ + { + "filename": "envelope.json", + "namespace": "canon.lint_rules.rust_makefile_baseline", + "successes": 13, + "failures": [ + { + "msg": "cannot prove the gate", + "metadata": { + "line": 0, + "path": "Makefile", + "rule_id": "QG-001", + "severity": "error", + "verdict": "indeterminate", + }, + } + ], + } + ]) cmd_mox.mock("conftest").returns(exit_code=1, stdout=doc) cmd_mox.replay() result = run_rule("rust-makefile-baseline", tmp_path) diff --git a/tests/unit/test_runtime.py b/tests/unit/test_runtime.py new file mode 100644 index 0000000..50a2555 --- /dev/null +++ b/tests/unit/test_runtime.py @@ -0,0 +1,77 @@ +"""Tests for the optional native runtime boundary.""" + +from __future__ import annotations + +import importlib +import types +import typing as typ + +import pytest + +import concordat +from concordat import pure, runtime + + +def test_runtime_uses_native_hello_when_extension_is_available( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The runtime selects the optional extension's implementation.""" + + def native_hello() -> str: + return "hello from Rust" + + native_module = types.SimpleNamespace(hello=native_hello) + try: + with monkeypatch.context() as context: + context.setattr(runtime.importlib, "import_module", lambda _: native_module) + reloaded_runtime = importlib.reload(runtime) + reloaded_concordat = importlib.reload(concordat) + + assert reloaded_runtime.hello is native_hello + assert reloaded_concordat.hello is native_hello + finally: + importlib.reload(runtime) + importlib.reload(concordat) + + +def test_runtime_falls_back_to_pure_hello_when_extension_is_missing( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The runtime retains a working Python implementation without the extension.""" + + def raise_module_not_found(_: str) -> typ.NoReturn: + raise ModuleNotFoundError(name="_concordat_rs") + + try: + with monkeypatch.context() as context: + context.setattr(runtime.importlib, "import_module", raise_module_not_found) + reloaded_runtime = importlib.reload(runtime) + reloaded_concordat = importlib.reload(concordat) + + assert reloaded_runtime.hello is pure.hello + assert reloaded_concordat.hello is pure.hello + finally: + importlib.reload(runtime) + importlib.reload(concordat) + + +def test_runtime_reraises_missing_native_dependency( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A native extension dependency failure must remain visible to callers.""" + error = ModuleNotFoundError(name="native_dependency") + + def raise_dependency_error(_: str) -> typ.NoReturn: + raise error + + try: + with monkeypatch.context() as context: + context.setattr(runtime.importlib, "import_module", raise_dependency_error) + + with pytest.raises(ModuleNotFoundError) as raised: + importlib.reload(runtime) + + assert raised.value is error + finally: + importlib.reload(runtime) + importlib.reload(concordat) diff --git a/uv.lock b/uv.lock index 2d067c7..1d1fb77 100644 --- a/uv.lock +++ b/uv.lock @@ -2,6 +2,15 @@ version = 1 revision = 3 requires-python = ">=3.13" +[[package]] +name = "astroid" +version = "4.0.4" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/07/63/0adf26577da5eff6eb7a177876c1cfa213856be9926a000f65c4add9692b/astroid-4.0.4.tar.gz", hash = "sha256:986fed8bcf79fb82c78b18a53352a0b287a73817d6dbcfba3162da36667c49a0", size = 406358, upload-time = "2026-02-07T23:35:07.509Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/b0/cf/1c5f42b110e57bc5502eb80dbc3b03d256926062519224835ef08134f1f9/astroid-4.0.4-py3-none-any.whl", hash = "sha256:52f39653876c7dec3e3afd4c2696920e05c83832b9737afc21928f2d2eb7a753", size = 276445, upload-time = "2026-02-07T23:35:05.344Z" }, +] + [[package]] name = "attrs" version = "25.4.0" @@ -172,6 +181,7 @@ dependencies = [ [package.dev-dependencies] dev = [ { name = "betamax" }, + { name = "df12-python-lints" }, { name = "hypothesis" }, { name = "pyright" }, { name = "pytest" }, @@ -198,6 +208,7 @@ requires-dist = [ [package.metadata.requires-dev] dev = [ { name = "betamax" }, + { name = "df12-python-lints", git = "https://github.com/leynos/df12-python-lints.git?rev=9c835f35b0f1690597ade799c9c6a30bc5922959" }, { name = "hypothesis" }, { name = "pyright" }, { name = "pytest" }, @@ -281,6 +292,23 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/97/49/f50d1ebb472902c952835d6141bfe920a16c402660a25c813151a861ca4c/cyclopts-4.22.4-py3-none-any.whl", hash = "sha256:90debd2468c5b33d7ca55ca64209a3bdf2667c11a2f75d973584198b58ca5e46", size = 234023, upload-time = "2026-08-02T14:04:16.282Z" }, ] +[[package]] +name = "df12-python-lints" +version = "0.1.0" +source = { git = "https://github.com/leynos/df12-python-lints.git?rev=9c835f35b0f1690597ade799c9c6a30bc5922959#9c835f35b0f1690597ade799c9c6a30bc5922959" } +dependencies = [ + { name = "pylint" }, +] + +[[package]] +name = "dill" +version = "0.4.1" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/81/e1/56027a71e31b02ddc53c7d65b01e68edf64dea2932122fe7746a516f75d5/dill-0.4.1.tar.gz", hash = "sha256:423092df4182177d4d8ba8290c8a5b640c66ab35ec7da59ccfa00f6fa3eea5fa", size = 187315, upload-time = "2026-01-19T02:36:56.85Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/1e/77/dc8c558f7593132cf8fefec57c4f60c83b16941c574ac5f619abb3ae7933/dill-0.4.1-py3-none-any.whl", hash = "sha256:1e1ce33e978ae97fcfcff5638477032b801c46c7c65cf717f95fbc2248f79a9d", size = 120019, upload-time = "2026-01-19T02:36:55.663Z" }, +] + [[package]] name = "docstring-parser" version = "0.17.0" @@ -396,6 +424,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/cb/b1/3846dd7f199d53cb17f49cba7e651e9ce294d8497c8c150530ed11865bb8/iniconfig-2.3.0-py3-none-any.whl", hash = "sha256:f631c04d2c48c52b84d0d0549c99ff3859c98df65b3101406327ecc7d53fbf12", size = 7484, upload-time = "2025-10-18T21:55:41.639Z" }, ] +[[package]] +name = "isort" +version = "8.0.1" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/ef/7c/ec4ab396d31b3b395e2e999c8f46dec78c5e29209fac49d1f4dace04041d/isort-8.0.1.tar.gz", hash = "sha256:171ac4ff559cdc060bcfff550bc8404a486fee0caab245679c2abe7cb253c78d", size = 769592, upload-time = "2026-02-28T10:08:20.685Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/3e/95/c7c34aa53c16353c56d0b802fba48d5f5caa2cdee7958acbcb795c830416/isort-8.0.1-py3-none-any.whl", hash = "sha256:28b89bc70f751b559aeca209e6120393d43fbe2490de0559662be7a9787e3d75", size = 89733, upload-time = "2026-02-28T10:08:19.466Z" }, +] + [[package]] name = "jmespath" version = "1.0.1" @@ -498,6 +535,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/70/bc/6f1c2f612465f5fa89b95bead1f44dcb607670fd42891d8fdcd5d039f4f4/markupsafe-3.0.3-cp314-cp314t-win_arm64.whl", hash = "sha256:32001d6a8fc98c8cb5c947787c5d08b0a50663d139f1305bac5885d98d9b40fa", size = 14146, upload-time = "2025-09-27T18:37:28.327Z" }, ] +[[package]] +name = "mccabe" +version = "0.7.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/e7/ff/0ffefdcac38932a54d2b5eed4e0ba8a408f215002cd178ad1df0f2806ff8/mccabe-0.7.0.tar.gz", hash = "sha256:348e0240c33b60bbdf4e523192ef919f28cb2c3d7d5c7794f74009290f236325", size = 9658, upload-time = "2022-01-24T01:14:51.113Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/27/1a/1f68f9ba0c207934b35b86a8ca3aad8395a3d6dd7921c0686e23853ff5a9/mccabe-0.7.0-py2.py3-none-any.whl", hash = "sha256:6c2d30ab6be0e4a46919781807b4f0d834ebdd6c6e3dca0bda5a15f863427b6e", size = 7350, upload-time = "2022-01-24T01:14:49.62Z" }, +] + [[package]] name = "mdit-py-plugins" version = "0.5.0" @@ -650,6 +696,24 @@ crypto = [ { name = "cryptography" }, ] +[[package]] +name = "pylint" +version = "4.0.7" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "astroid" }, + { name = "colorama", marker = "sys_platform == 'win32'" }, + { name = "dill" }, + { name = "isort" }, + { name = "mccabe" }, + { name = "platformdirs" }, + { name = "tomlkit" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/de/92/98dace02f2d11b88160354c53944f77ea7327aa78bce1c75971e7aaa4347/pylint-4.0.7.tar.gz", hash = "sha256:9b2d1d15791c84b77a4fe2aafe8f0d9570717e2dea06d53b19c105cf60275a52", size = 1594770, upload-time = "2026-08-09T19:13:23.289Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/e0/b0/3a8040e53df6c5c1e04b0e23ed53fdbeb64f333723a334d313fba2f581ce/pylint-4.0.7-py3-none-any.whl", hash = "sha256:be4a3111557a614411ed1fc89347ce4a8e1013a59e1f33d11485227a02e3304d", size = 539710, upload-time = "2026-08-09T19:13:21.228Z" }, +] + [[package]] name = "pyright" version = "1.1.411" @@ -889,6 +953,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/0b/66/19ec715043ab3753f3a36252d72d5d492cbdf7fda67e69637a38774d45f6/tofupy-1.1.2-py3-none-any.whl", hash = "sha256:facdb343d9ab39ec6e408cbf3562ed5b1553032a9043d9c97c9901ba88fac1a7", size = 22595, upload-time = "2025-12-05T17:58:32.674Z" }, ] +[[package]] +name = "tomlkit" +version = "0.15.1" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/94/96/e07752635b98536177fa1f37671c8f3cdde2e724c6bcf6034b2cfb571565/tomlkit-0.15.1.tar.gz", hash = "sha256:e25bbf38843005246210a12982776f27f99cb9be67160e14434d0c0d21ee1e97", size = 180129, upload-time = "2026-07-17T01:48:04.562Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/13/bc/8c13eb66537dce1d2bd3a57132902f38d0e7f5bb46fa9f4daed9fe9d76ee/tomlkit-0.15.1-py3-none-any.whl", hash = "sha256:177a05aece5a8ca5266fd3c448abb47b8d352f09d477d3ca8332db4d89b24304", size = 49449, upload-time = "2026-07-17T01:48:05.728Z" }, +] + [[package]] name = "typing-extensions" version = "4.15.0"