From 33999f97a2f42903994738e7dc3285a6ab3fa0ec Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Wed, 2 Sep 2026 23:40:46 -0400 Subject: [PATCH] feat(stores): route audit-cache through AuditCacheStore protocol (feature 035) Close the wiring gap where tools/audit.py::run_sieve_audit called core.audit_cache.write_audit_cache directly, ignoring feature 033's execution_context.stores.cache bundle. Operators setting [stores.cache] backend/root in .baseline.toml now see the audit-cache actually redirect. Core changes: - core/audit_cache.py rewritten as thin wrapper over AuditCacheStore. Public API keeps positional signatures (backward compat) plus two new kwargs (store, cache_key) for driver-side calls. TTL / git-HEAD / working-tree-dirty staleness enforcement stays in the wrapper, not the store. - tools/audit.py picks cache_key by whether stores_config.cache is set: "audit-cache" for the default-store case (root already encodes repo identity), sha256(abspath(repo))[:16] for the configured-store case (per-repo isolation under a shared root). - stores/selection.py default cache_root moved from /.darnit/ audit-cache/ to /darnit/ so bundle.cache's default backend produces the same byte-for-byte on-disk path as the pre-feature audit_cache wrapper. Behavior changes documented in the spec: - write_audit_cache is best-effort per feature 033 FR-011 (log and continue; previously raised on tempfile failure). - invalidate_audit_cache writes an expired envelope (timestamp 1970-01-01T00:00:00Z) rather than deleting the file. The AuditCacheStore protocol has no delete(key) method; the surface stays unchanged. Tests: - 11 new driver-level integration tests at tests/darnit/ test_audit_cache_store_wiring.py cover configured-store routing, zero-config invariance, staleness through a configured store, shared -root non-collision, fault injection, and backward-compat call form. - Two existing tests updated for behavior change: no-raise on write failure, invalidate leaves an expired envelope. - Two zero-config path-shape tests updated to expect the new default cache_root shape (tempdir/hash instead of repo/.darnit/audit-cache). Verified end-to-end: darnit audit with [stores.cache] backend="local-fs" root="/tmp/qs-cache" writes /tmp/qs-cache/.json and does NOT create a fallback file under the system tempdir. Full framework test suite: 1900 passed, 6 skipped, 0 failed. --- .specify/feature.json | 2 +- CLAUDE.md | 2 +- .../darnit/src/darnit/core/audit_cache.py | 239 ++++++++--- .../darnit/src/darnit/stores/selection.py | 36 +- packages/darnit/src/darnit/tools/audit.py | 43 +- .../checklists/requirements.md | 36 ++ .../contracts/audit-cache-wrapper.md | 126 ++++++ .../data-model.md | 132 ++++++ specs/035-audit-cache-store-migration/plan.md | 165 ++++++++ .../quickstart.md | 107 +++++ .../research.md | 79 ++++ specs/035-audit-cache-store-migration/spec.md | 181 ++++++++ .../035-audit-cache-store-migration/tasks.md | 213 ++++++++++ tests/darnit/core/test_audit_cache.py | 38 +- tests/darnit/stores/test_us1_isolation.py | 12 +- tests/darnit/stores/test_us2_zero_config.py | 24 +- tests/darnit/test_audit_cache_store_wiring.py | 396 ++++++++++++++++++ 17 files changed, 1709 insertions(+), 122 deletions(-) create mode 100644 specs/035-audit-cache-store-migration/checklists/requirements.md create mode 100644 specs/035-audit-cache-store-migration/contracts/audit-cache-wrapper.md create mode 100644 specs/035-audit-cache-store-migration/data-model.md create mode 100644 specs/035-audit-cache-store-migration/plan.md create mode 100644 specs/035-audit-cache-store-migration/quickstart.md create mode 100644 specs/035-audit-cache-store-migration/research.md create mode 100644 specs/035-audit-cache-store-migration/spec.md create mode 100644 specs/035-audit-cache-store-migration/tasks.md create mode 100644 tests/darnit/test_audit_cache_store_wiring.py diff --git a/.specify/feature.json b/.specify/feature.json index c600a102..3db53393 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1 +1 @@ -{"feature_directory": "specs/034-local-output-store"} +{"feature_directory": "specs/035-audit-cache-store-migration"} diff --git a/CLAUDE.md b/CLAUDE.md index 50583300..97bb88d3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -381,5 +381,5 @@ else: For additional context about technologies to be used, project structure, shell commands, and other important information, read the current plan: -[`specs/034-local-output-store/plan.md`](specs/034-local-output-store/plan.md) +[`specs/035-audit-cache-store-migration/plan.md`](specs/035-audit-cache-store-migration/plan.md) diff --git a/packages/darnit/src/darnit/core/audit_cache.py b/packages/darnit/src/darnit/core/audit_cache.py index 08076a50..fe2d3f21 100644 --- a/packages/darnit/src/darnit/core/audit_cache.py +++ b/packages/darnit/src/darnit/core/audit_cache.py @@ -1,51 +1,94 @@ """Audit result cache for cross-tool-call persistence. -Caches audit results in a temp directory so that the remediate tool can -skip re-running the audit when results are fresh. Each repository gets -its own cache keyed by a short hash of its absolute path: - - $TMPDIR/darnit//audit-cache.json - -Staleness is tracked via the git HEAD commit hash and working-tree -dirty state. Any mismatch → cache miss → remediate falls back to -running a fresh audit. - -Public API: - write_audit_cache(local_path, results, summary, level, framework) - read_audit_cache(local_path) -> dict | None - invalidate_audit_cache(local_path) +Caches audit results so the remediate tool can skip re-running the audit +when results are fresh. Feature 035 refactored this module from a +direct-tempdir implementation into a thin wrapper over feature 033's +:class:`~darnit.stores.protocols.AuditCacheStore` Protocol, so an +operator setting ``[stores.cache]`` in ``.baseline.toml`` actually +redirects the cache location. + +Two call forms: + +* **Backward-compat** (external callers, no store passed): the wrapper + builds a :class:`~darnit.stores.defaults.FilesystemAuditCacheStore` + rooted at ``/darnit/`` and uses + the literal cache key ``"audit-cache"``. On-disk path is byte-for-byte + identical to the pre-feature layout:: + + /darnit//audit-cache.json + +* **Driver call form** (:mod:`darnit.tools.audit` passes both ``store`` + and ``cache_key``): the wrapper uses them verbatim. The driver decides + the cache_key based on whether ``[stores.cache]`` was configured -- + ``"audit-cache"`` for the default store (root already encodes repo + identity) or ``sha256(abspath(repo))[:16]`` for a configured store + (root is operator-picked, key must encode repo identity). + +Staleness is tracked via TTL, git HEAD commit hash, and working-tree +dirty state -- all in this wrapper, not in the store. The store is a +plain KV over dict envelopes. + +Public API:: + + write_audit_cache(local_path, results, summary, level, framework, + *, store=None, cache_key=None) + read_audit_cache(local_path, ttl_seconds=3600, + *, store=None, cache_key=None) -> dict | None + invalidate_audit_cache(local_path, *, store=None, cache_key=None) """ from __future__ import annotations import hashlib -import json -import os import subprocess import tempfile from datetime import UTC, datetime from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any from darnit.core.logging import get_logger from darnit.sieve.models import CheckResult +if TYPE_CHECKING: + from darnit.stores.protocols import AuditCacheStore + logger = get_logger("core.audit_cache") CACHE_FILENAME = "audit-cache.json" CACHE_VERSION = 1 +# Invalidation via write-expired-envelope (feature 035 clarify Q3). +# The AuditCacheStore Protocol has no delete(key) method; instead, +# invalidate_audit_cache overwrites the current envelope with one whose +# timestamp is guaranteed to fail every future TTL check. +_EXPIRED_ENVELOPE: dict[str, Any] = { + "version": CACHE_VERSION, + "timestamp": "1970-01-01T00:00:00+00:00", + "commit": None, + "commit_dirty": False, + "level": 0, + "framework": "", + "results": [], + "summary": {}, +} + # --------------------------------------------------------------------------- -# Cache location +# Cache location (backward-compat helper) # --------------------------------------------------------------------------- def _get_cache_dir(local_path: str) -> Path: """Return the per-repo cache directory under the system temp dir. + Used by: + * the backward-compat default-store construction in the three public + wrapper functions when the caller does NOT pass ``store``, and + * existing tests at ``tests/darnit/core/test_audit_cache.py`` that + compose the expected on-disk path via this helper. + Uses a short SHA-256 hash of the repo's resolved absolute path so - that each repository gets an isolated cache directory. + each repository gets an isolated cache directory. """ resolved = str(Path(local_path).resolve()) repo_hash = hashlib.sha256(resolved.encode()).hexdigest()[:16] @@ -88,10 +131,42 @@ def _is_working_tree_dirty(local_path: str) -> bool: return len(result.stdout.strip()) > 0 except (subprocess.TimeoutExpired, FileNotFoundError, OSError): pass - # If we can't determine dirty state, assume dirty (conservative) + # If we can't determine dirty state, assume dirty (conservative). return True +# --------------------------------------------------------------------------- +# Internal store selection +# --------------------------------------------------------------------------- + + +def _resolve_store_and_key( + local_path: str, + store: AuditCacheStore | None, + cache_key: str | None, +) -> tuple[AuditCacheStore, str]: + """Return ``(store, cache_key)`` per the two-form contract. + + * Both ``None``: build a default + :class:`~darnit.stores.defaults.FilesystemAuditCacheStore` rooted at + ``/darnit/`` and use + cache_key ``"audit-cache"``. Byte-for-byte legacy path. + * Both provided: return as-is. + * Exactly one provided: :class:`TypeError` (partial-kwarg guardrail + per contracts/audit-cache-wrapper.md). + """ + if (store is None) != (cache_key is None): + raise TypeError("store and cache_key must be passed together") + if store is None: + # Deferred import to keep the module import graph shallow. + from darnit.stores.defaults import FilesystemAuditCacheStore + + store = FilesystemAuditCacheStore(_get_cache_dir(local_path)) + cache_key = "audit-cache" + assert cache_key is not None # narrow type + return store, cache_key + + # --------------------------------------------------------------------------- # Public API # --------------------------------------------------------------------------- @@ -103,21 +178,34 @@ def write_audit_cache( summary: dict[str, int], level: int, framework: str, + *, + store: AuditCacheStore | None = None, + cache_key: str | None = None, ) -> None: - """Write audit results to the cache. + """Write audit results to the cache via the given (or default) store. - Creates the cache directory if it doesn't exist. Uses atomic write - (tempfile + rename) to prevent corruption from interrupted writes. + Composes the cache envelope (version, timestamp, commit, commit_dirty, + level, framework, results, summary) and hands it to + ``store.write(cache_key, envelope)``. Failures are logged at warning + level and swallowed -- per feature 035 FR-007, cache-write failure + MUST NOT propagate to the audit's exit code. Args: - local_path: Path to the repository root. + local_path: Path to the repository root (used for git + introspection and, when ``store`` is None, for default-store + construction). results: The raw results list from ``run_sieve_audit()``. summary: Status count summary from ``run_sieve_audit()``. level: Maximum audit level that was evaluated. framework: Framework name (e.g. ``"openssf-baseline"``). + store: Optional :class:`AuditCacheStore` instance. When None, a + legacy-path default store is constructed. Must be passed + together with ``cache_key``. + cache_key: Optional cache key string. When None, defaults to + ``"audit-cache"`` (used with the default store's per-repo + tempdir root). Must be passed together with ``store``. """ - cache_dir = _get_cache_dir(local_path) - cache_dir.mkdir(parents=True, exist_ok=True) + store, cache_key = _resolve_store_and_key(local_path, store, cache_key) envelope: dict[str, Any] = { "version": CACHE_VERSION, @@ -130,47 +218,46 @@ def write_audit_cache( "summary": summary, } - cache_path = cache_dir / CACHE_FILENAME - - # Atomic write: write to temp file in same directory, then rename. - fd, tmp_path = tempfile.mkstemp(dir=str(cache_dir), suffix=".tmp", prefix="audit-cache-") try: - with os.fdopen(fd, "w", encoding="utf-8") as f: - json.dump(envelope, f, indent=2) - os.replace(tmp_path, str(cache_path)) - logger.debug("Wrote audit cache to %s", cache_path) - except Exception: - # Clean up temp file on failure - try: - os.unlink(tmp_path) - except OSError: - pass - raise + store.write(cache_key, envelope) + except Exception as exc: # noqa: BLE001 -- FR-007 best-effort + logger.warning( + "audit cache write failed (non-fatal): %s: %s", + type(exc).__name__, + exc, + ) -def read_audit_cache(local_path: str, ttl_seconds: int = 3600) -> dict[str, Any] | None: +def read_audit_cache( + local_path: str, + ttl_seconds: int = 3600, + *, + store: AuditCacheStore | None = None, + cache_key: str | None = None, +) -> dict[str, Any] | None: """Read cached audit results if they are still fresh. - Returns the full cache envelope (including ``results`` and ``summary``) - when the cache exists, is valid JSON, has a supported version, - is within the TTL limit, and the git commit + dirty state match - the current repository state. + Returns the full cache envelope (including ``results`` and + ``summary``) when the cache exists, has a supported version, is + within the TTL, and its git commit + dirty state match the current + repository state. + + Returns ``None`` on any mismatch, missing file, or corruption -- + callers should fall back to running a fresh audit. Never raises. - Returns ``None`` on any mismatch, missing file, or corruption — - callers should fall back to running a fresh audit. + Staleness enforcement (TTL / commit / dirty) lives here, NOT in the + store; the store is a plain KV. """ - cache_path = _get_cache_dir(local_path) / CACHE_FILENAME + store, cache_key = _resolve_store_and_key(local_path, store, cache_key) - if not cache_path.is_file(): - logger.debug("No audit cache file at %s", cache_path) + try: + data = store.read(cache_key) + except Exception as exc: # noqa: BLE001 -- read must never raise + logger.debug("audit cache read failed: %s", exc) return None - # Parse JSON - try: - with open(cache_path, encoding="utf-8") as f: - data = json.load(f) - except (json.JSONDecodeError, OSError) as exc: - logger.debug("Corrupt or unreadable audit cache: %s", exc) + if data is None: + logger.debug("No audit cache found for key %s", cache_key) return None if not isinstance(data, dict): @@ -201,8 +288,8 @@ def read_audit_cache(local_path: str, ttl_seconds: int = 3600) -> dict[str, Any] # Staleness: commit hash cached_commit = data.get("commit") if cached_commit is None: - # Written in a non-git repo → always stale - logger.debug("Audit cache has null commit — treating as stale") + # Written in a non-git repo -> always stale. + logger.debug("Audit cache has null commit -- treating as stale") return None current_commit = _get_head_commit(local_path) @@ -229,14 +316,32 @@ def read_audit_cache(local_path: str, ttl_seconds: int = 3600) -> dict[str, Any] return data -def invalidate_audit_cache(local_path: str) -> None: - """Delete the audit cache file if it exists. +def invalidate_audit_cache( + local_path: str, + *, + store: AuditCacheStore | None = None, + cache_key: str | None = None, +) -> None: + """Invalidate the audit cache by writing an expired envelope. + + Feature 035 clarify Q3: since the :class:`AuditCacheStore` Protocol + has no ``delete(key)`` method, invalidation is implemented as an + overwrite with an envelope whose timestamp is 1970-01-01T00:00:00Z. + The next :func:`read_audit_cache` misses on the TTL check. + + The on-disk cache file remains until the next successful write + overwrites it -- acceptable because the default cache path is an + operator-invisible tempdir. - No-op if the file is already missing. + Never raises. Write failures are logged at warning level and + swallowed (same contract as :func:`write_audit_cache`). """ - cache_path = _get_cache_dir(local_path) / CACHE_FILENAME + store, cache_key = _resolve_store_and_key(local_path, store, cache_key) try: - cache_path.unlink(missing_ok=True) - logger.debug("Invalidated audit cache at %s", cache_path) - except OSError as exc: - logger.debug("Could not remove audit cache: %s", exc) + store.write(cache_key, dict(_EXPIRED_ENVELOPE)) + except Exception as exc: # noqa: BLE001 + logger.warning( + "audit cache invalidation failed (non-fatal): %s: %s", + type(exc).__name__, + exc, + ) diff --git a/packages/darnit/src/darnit/stores/selection.py b/packages/darnit/src/darnit/stores/selection.py index f923fede..88de913d 100644 --- a/packages/darnit/src/darnit/stores/selection.py +++ b/packages/darnit/src/darnit/stores/selection.py @@ -24,6 +24,8 @@ class never constructs its store). from __future__ import annotations +import hashlib +import tempfile from collections.abc import Callable from pathlib import Path from typing import Any @@ -134,7 +136,10 @@ def resolve_stores( need it. attestation_root, report_root, cache_root: Optional overrides for filesystem-default roots. When None, uses - ``/.darnit/{attestations,reports,audit-cache}``. + ``/.darnit/{attestations,reports}`` for the first two + and ``/darnit/`` + for the cache (feature 035: matches the pre-feature + :mod:`darnit.core.audit_cache` on-disk path). Returns: A :class:`_StoreBundle` whose fields lazily instantiate on first @@ -148,7 +153,14 @@ def resolve_stores( """ attestation_root = attestation_root or (repo_path / ".darnit" / "attestations") report_root = report_root or (repo_path / ".darnit" / "reports") - cache_root = cache_root or (repo_path / ".darnit" / "audit-cache") + if cache_root is None: + # Feature 035: default cache lands at the legacy per-repo tempdir path + # (/darnit/) so bundle.cache's + # default backend produces the same on-disk path as the pre-feature + # audit_cache wrapper. The wrapper passes cache_key = "audit-cache" + # in the default-store case, yielding /audit-cache.json. + _repo_hash = hashlib.sha256(str(repo_path.resolve()).encode()).hexdigest()[:16] + cache_root = Path(tempfile.gettempdir()) / "darnit" / _repo_hash default_factories = { "project": lambda: FilesystemProjectStateStore(repo_path), @@ -163,16 +175,12 @@ def resolve_stores( if block is None: factories[kind] = default_factories[kind] else: - factories[kind] = _validate_and_make_factory( - kind, block, repo_path=repo_path - ) + factories[kind] = _validate_and_make_factory(kind, block, repo_path=repo_path) return _StoreBundle(factories) -def _validate_and_make_factory( - kind: str, block: Any, *, repo_path: Path -) -> Callable[[], Any]: +def _validate_and_make_factory(kind: str, block: Any, *, repo_path: Path) -> Callable[[], Any]: """Discover the plugin, validate its class shape, return a factory. Validation runs eagerly (before the factory fires) so a bad @@ -192,11 +200,7 @@ def _validate_and_make_factory( cls = registered[name] # Class-shape Protocol check (avoids instantiation). - missing = [ - attr - for attr in _protocol_methods(protocol_cls) - if not hasattr(cls, attr) - ] + missing = [attr for attr in _protocol_methods(protocol_cls) if not hasattr(cls, attr)] if missing: raise StoreProtocolMismatch( group=group, @@ -205,11 +209,7 @@ def _validate_and_make_factory( missing=missing, ) - kwargs = { - k: v - for k, v in dict(block.model_extra or {}).items() - if k != "backend" - } + kwargs = {k: v for k, v in dict(block.model_extra or {}).items() if k != "backend"} kwargs.setdefault("repo_path", repo_path) def _factory() -> Any: diff --git a/packages/darnit/src/darnit/tools/audit.py b/packages/darnit/src/darnit/tools/audit.py index f85576a3..be89e7e7 100644 --- a/packages/darnit/src/darnit/tools/audit.py +++ b/packages/darnit/src/darnit/tools/audit.py @@ -8,6 +8,7 @@ and can be used for programmatic access. """ +import hashlib from dataclasses import dataclass from pathlib import Path from typing import Any @@ -144,9 +145,7 @@ def _get_framework_config_path(framework_name: str | None = None) -> Path | None return None -def _load_merged_stores( - local_path: str, framework_name: str | None -) -> Any: +def _load_merged_stores(local_path: str, framework_name: str | None) -> Any: """Return the merged ``StoresConfig`` for this audit run. Feature 033. Composes the framework TOML's ``[stores]`` block with @@ -170,9 +169,7 @@ def _load_merged_stores( return getattr(effective, "stores", None) -def _load_merged_mcp_servers( - local_path: str, framework_name: str | None -) -> dict[str, Any]: +def _load_merged_mcp_servers(local_path: str, framework_name: str | None) -> dict[str, Any]: """Return the merged ``mcp_servers`` allowlist for this audit run. Composes the framework TOML's block with any ``.baseline.toml`` @@ -537,9 +534,7 @@ def run_sieve_audit( # simply see an empty allowlist and any mcp handler pass resolves # ERROR ("unknown MCP server: ...") at dispatch time. try: - execution_context.mcp_servers = _load_merged_mcp_servers( - local_path, resolved_fw - ) + execution_context.mcp_servers = _load_merged_mcp_servers(local_path, resolved_fw) except Exception as err: # noqa: BLE001 - config load must not break audit logger.debug("MCP allowlist load failed (non-fatal): %s", err) all_results: list[CheckResult] = [] @@ -662,11 +657,33 @@ def run_sieve_audit( summary = summarize_results(all_results) # Write results to cache so remediate can skip re-running the audit. - # Failures here must never break the audit pipeline. - try: - from darnit.core.audit_cache import write_audit_cache + # Feature 035: route through stores_bundle.cache so [stores.cache] TOML + # config actually redirects the on-disk location. Pick the cache_key + # by whether the operator configured a store: default store's root + # already encodes repo identity (uses fixed "audit-cache"); configured + # store's root is operator-picked and may be shared across repos, so + # the key must encode repo identity. + if stores_config is None or stores_config.cache is None: + cache_key = "audit-cache" + else: + cache_key = hashlib.sha256(str(Path(local_path).resolve()).encode()).hexdigest()[:16] + + from darnit.core.audit_cache import write_audit_cache - write_audit_cache(local_path, all_results, summary, level, resolved_fw or "") + # write_audit_cache is best-effort per FR-007 -- it swallows and logs + # internally. The enclosing try/except is defensive belt-and-braces + # against any unexpected exception path (e.g. TypeError from a + # future signature change). + try: + write_audit_cache( + local_path, + all_results, + summary, + level, + resolved_fw or "", + store=stores_bundle.cache, + cache_key=cache_key, + ) except Exception as exc: logger.warning("Failed to write audit cache (non-fatal): %s", exc) diff --git a/specs/035-audit-cache-store-migration/checklists/requirements.md b/specs/035-audit-cache-store-migration/checklists/requirements.md new file mode 100644 index 00000000..3ea77f77 --- /dev/null +++ b/specs/035-audit-cache-store-migration/checklists/requirements.md @@ -0,0 +1,36 @@ +# Specification Quality Checklist: Audit-Cache Store Migration + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-09-02 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [X] No implementation details (languages, frameworks, APIs) -- names like `bundle.cache` and `AuditCacheStore` refer to the feature-033 vocabulary the operator sees in config and error messages, not to implementation choices +- [X] Focused on user value and business needs -- CI operator, upgrading user, correctness invariants +- [X] Written for non-technical stakeholders -- section headings describe outcomes, not code +- [X] All mandatory sections completed + +## Requirement Completeness + +- [X] No [NEEDS CLARIFICATION] markers remain -- 3 open design questions surfaced in spec's Assumptions / Out of Scope / FR-009 that clarify pass will formalize as questions rather than markers +- [X] Requirements are testable and unambiguous -- every FR is a MUST/MUST NOT with a verifiable behavior +- [X] Success criteria are measurable +- [X] Success criteria are technology-agnostic (no implementation details) -- SCs describe cache-hit/miss and file locations +- [X] All acceptance scenarios are defined -- 4 stories with Given/When/Then coverage +- [X] Edge cases are identified -- 6 edge cases enumerated +- [X] Scope is clearly bounded -- explicit Out of Scope section names 6 non-goals +- [X] Dependencies and assumptions identified + +## Feature Readiness + +- [X] All functional requirements have clear acceptance criteria -- FR-001..014 each mappable to an SC or acceptance scenario +- [X] User scenarios cover primary flows -- 4 prioritized stories including a P2 backward-compat invariant +- [X] Feature meets measurable outcomes defined in Success Criteria +- [X] No implementation details leak into specification + +## Notes + +- Passed on first draft. Two design questions are surfaced ambiguously in the spec (default cache location; whether `AuditCacheStore.delete()` is added); clarify pass will formalize both. +- The FR-007 behavior change ("write raises" -> "write logs and continues") is intentional and worth flagging in the plan phase's Constitution Check (Principle II Conservative-by-Default is respected because a cache failure never makes an audit report incorrect; it just makes the next run slower). +- The default-location question is the biggest user-visible risk. If we move from `$TMPDIR` to `/.darnit/audit-cache/`, we owe operators .gitignore guidance and an upgrade note. diff --git a/specs/035-audit-cache-store-migration/contracts/audit-cache-wrapper.md b/specs/035-audit-cache-store-migration/contracts/audit-cache-wrapper.md new file mode 100644 index 00000000..4e38243e --- /dev/null +++ b/specs/035-audit-cache-store-migration/contracts/audit-cache-wrapper.md @@ -0,0 +1,126 @@ +# Contract: `darnit.core.audit_cache` wrapper (post-feature-035) + +**Feature**: 035-audit-cache-store-migration + +This contract enumerates the observable behavior every consumer of `write_audit_cache` / `read_audit_cache` / `invalidate_audit_cache` may rely on after this feature ships. Anything not stated here is unspecified. + +## 1. Public signatures + +```python +def write_audit_cache( + local_path: str, + results: list[CheckResult], + summary: dict[str, int], + level: int, + framework: str, + *, + store: AuditCacheStore | None = None, + cache_key: str | None = None, +) -> None: ... + +def read_audit_cache( + local_path: str, + ttl_seconds: int = 3600, + *, + store: AuditCacheStore | None = None, + cache_key: str | None = None, +) -> dict[str, Any] | None: ... + +def invalidate_audit_cache( + local_path: str, + *, + store: AuditCacheStore | None = None, + cache_key: str | None = None, +) -> None: ... +``` + +### 1.1 Backward-compat call form + +An external caller who invokes any of these three functions passing only positional args (no `store=`, no `cache_key=`) MUST observe the same on-disk cache path as the pre-feature implementation: + +```text +/darnit//audit-cache.json +``` + +...where `` is `tempfile.gettempdir()`. Byte-for-byte identical to today's `_get_cache_dir(...) / CACHE_FILENAME`. + +### 1.2 Driver call form + +`tools/audit.py::run_sieve_audit` MUST call the wrapper with both `store=` and `cache_key=` provided. See section 4. + +### 1.3 Partial-kwarg call is an error + +If exactly one of `store` and `cache_key` is `None`, the wrapper raises `TypeError("store and cache_key must be passed together")`. This catches driver bugs at call time rather than silently defaulting. + +## 2. Cache-key composition rule + +* **Default-store case**: literal string `"audit-cache"`. The store's root already encodes repo identity via the wrapper's default-store construction. Final on-disk path: `/darnit//audit-cache.json`. +* **Configured-store case**: `sha256(abspath(local_path).encode()).hexdigest()[:16]`. The store's root is operator-picked and may be shared across repos; per-repo isolation lives in the key. Final on-disk path: `/.json`. + +## 3. Staleness enforcement (wrapper, not store) + +The wrapper MUST enforce these staleness checks on read, in this order. First `None`-returning check short-circuits. + +1. **Missing / corrupt / non-dict**: `store.read(cache_key)` returned `None`, or the returned value isn't a `dict`. Wrapper returns `None`. +2. **Version check**: envelope's `version` is missing, not an `int`, or greater than `CACHE_VERSION` (currently 1). Wrapper returns `None`. +3. **TTL**: envelope's `timestamp` is missing, unparseable, or more than `ttl_seconds` in the past (default 3600). Wrapper returns `None`. +4. **Null commit**: envelope's `commit` is `None` (means it was written in a non-git repo -- treat as always stale to force a fresh audit). Wrapper returns `None`. +5. **Commit mismatch**: envelope's `commit` differs from the current git HEAD of `local_path`. Wrapper returns `None`. +6. **Dirty-state mismatch**: envelope's `commit_dirty` differs from the current working-tree dirty state. Wrapper returns `None`. + +If all checks pass, wrapper returns the full envelope dict. Callers may inspect `envelope["results"]`, `envelope["summary"]`, etc. + +**Store MUST NOT know about TTL, commit, or dirty state.** Those are wrapper concerns. The store is a plain read-through / write-through KV over `dict[str, Any]`. + +## 4. Driver call-site rule (`tools/audit.py`) + +`run_sieve_audit` MUST call `write_audit_cache` (and mirror for `read_audit_cache` if a read call site is added later) with both `store` and `cache_key` derived from local scope: + +```python +if stores_config is None or stores_config.cache is None: + cache_key = "audit-cache" +else: + cache_key = hashlib.sha256( + str(Path(local_path).resolve()).encode() + ).hexdigest()[:16] + +write_audit_cache( + local_path, all_results, summary, level, resolved_fw or "", + store=stores_bundle.cache, + cache_key=cache_key, +) +``` + +The `stores_bundle.cache` property fires the lazy factory on first access; feature 033's `is_instantiated("cache")` becomes True after this call. Feature 033 SC-004 (no ghost construction) is preserved for any audit path that never reaches this line -- e.g., early-exit failure modes. + +## 5. Error semantics + +### 5.1 `write_audit_cache` + +MUST NOT raise on backend failure. If `store.write(cache_key, envelope)` raises (which the default `FilesystemAuditCacheStore` avoids -- it catches `OSError` internally and logs a warning), the wrapper catches and logs at warning level, then returns normally. + +**Change from pre-feature**: the previous implementation raised `OSError` on `tempfile.mkstemp` / `os.replace` failure. Post-feature 035, the audit run completes successfully even when the cache write fails. This is documented in the release notes as an intentional relaxation; the fault-injection test at `TestAtomicWrite::test_no_partial_file_on_error` is updated to expect no-raise. + +### 5.2 `read_audit_cache` + +MUST NOT raise on any failure mode (missing file, corrupt JSON, unreadable, non-dict, git command failure, etc.). Returns `None` for all failure modes. + +### 5.3 `invalidate_audit_cache` + +MUST NOT raise. Writes an expired-envelope via `store.write(cache_key, EXPIRED_ENVELOPE)`. If that write fails, logs a warning and returns. Subsequent reads on a still-fresh cache file will fail the TTL check anyway once the expired envelope lands; if the write itself failed, subsequent reads may succeed on the pre-invalidation cache -- caller MUST NOT rely on invalidation being observable in the face of write failure. + +## 6. Concurrency + +Two darnit processes writing to the same `cache_key` at the same time is the store's problem. `FilesystemAuditCacheStore.write` uses tempfile-then-rename for atomicity; the "loser" of the race silently overwrites the "winner." Acceptable per feature 033 FR-011 / best-effort contract. + +The wrapper itself is stateless -- no locks, no shared mutable state. + +## 7. Test surface + +Tests validating this contract live at: + +* `tests/darnit/core/test_audit_cache.py` (existing; 2 tests adjusted per research.md R-002). +* `tests/darnit/test_audit_cache_store_wiring.py` (new; locks driver-level integration per SC-001 / SC-006). +* `tests/darnit/stores/test_us2_zero_config.py::test_filesystem_defaults_use_canonical_darnit_paths` (feature 033 test; 1 assertion updated per research.md R-003). + +No plugin-package or implementation-package tests touched. diff --git a/specs/035-audit-cache-store-migration/data-model.md b/specs/035-audit-cache-store-migration/data-model.md new file mode 100644 index 00000000..43c4cffc --- /dev/null +++ b/specs/035-audit-cache-store-migration/data-model.md @@ -0,0 +1,132 @@ +# Phase 1: Data Model -- Audit-Cache Store Migration + +**Feature**: 035-audit-cache-store-migration | **Date**: 2026-09-02 + +No new database schema; this is a code-shape change. The "entities" here are the concrete values and objects flowing through the migrated code path. + +## E-001: Cache envelope (unchanged) + +The JSON blob written to disk. Structure is identical to the pre-feature `write_audit_cache` output, guaranteed byte-for-byte compatibility with existing on-disk caches. + +```python +{ + "version": int, # CACHE_VERSION = 1 + "timestamp": str, # datetime.now(UTC).isoformat() + "commit": str | None, # git HEAD (None when repo is not a git repo) + "commit_dirty": bool, # True if working tree has uncommitted changes + "level": int, # Max audit level evaluated + "framework": str, # Framework name (e.g. "openssf-baseline") + "results": list[dict], # Serialized CheckResults + "summary": dict[str, int], # Status counts +} +``` + +**Invariant**: The envelope is the same shape in both storage paths (default tempdir and configured-root). The store just handles bytes; the wrapper composes and interprets the shape. + +## E-002: Cache key + +The `cache_key` string the wrapper passes into `AuditCacheStore.read(cache_key)` and `.write(cache_key, envelope)`. Composition depends on which store the wrapper is talking to: + +**Default-store case** (`stores_config.cache is None`): `cache_key = "audit-cache"`. The store's root already encodes repo identity (`/darnit/`), so the key just names the file. Final path: `/darnit//audit-cache.json`. Byte-for-byte identical to pre-feature. + +**Configured-store case** (`stores_config.cache is not None`): `cache_key = sha256(abspath(repo_path))[:16]`. The store's root is operator-picked and shared across repos, so the key MUST encode repo identity. Final path: `/.json`. + +**Rationale**: Two paths, one wrapper. The driver decides which case it's in and passes the right key. The wrapper is dumb: it does not know or care about `stores_config`. + +**Hash function**: `sha256(abspath(repo_path).encode()).hexdigest()[:16]` -- identical to the pre-feature `_get_cache_dir` composition. Preserves per-repo stability across runs. + +## E-003: Store instance (`bundle.cache`) + +An `AuditCacheStore` Protocol implementer. Exposes exactly three methods: + +```python +class AuditCacheStore(Protocol): + def read(self, cache_key: str) -> dict[str, Any] | None: ... + def write(self, cache_key: str, envelope: dict[str, Any]) -> None: ... + def close(self) -> None: ... +``` + +Neither the Protocol nor any implementation changes in this feature. Only the DEFAULT ROOT that `resolve_stores` passes to `FilesystemAuditCacheStore` moves from `/.darnit/audit-cache/` to `/darnit/`. + +**Best-effort contract**: `read` returns `None` on any failure (missing, corrupt, unreadable). `write` swallows all `OSError` and logs a warning. Feature 033 FR-011. + +## E-004: Expired envelope (for invalidation) + +A cache envelope whose timestamp is guaranteed to be older than any reasonable TTL, so the next `read_audit_cache` returns `None` via the existing TTL check. + +```python +EXPIRED_ENVELOPE = { + "version": 1, + "timestamp": "1970-01-01T00:00:00Z", + "commit": None, + "commit_dirty": False, + "level": 0, + "framework": "", + "results": [], + "summary": {}, +} +``` + +`invalidate_audit_cache(local_path)` calls `store.write(cache_key, EXPIRED_ENVELOPE)`. The file remains on disk (Q3 answer: acceptable, cache path is operator-invisible tempdir), but every read after invalidation misses on TTL until a fresh write overwrites it. No `AuditCacheStore.delete()` method is added. + +## E-005: Wrapper API (public) + +Three functions, backward-compatible signatures plus two optional kwargs each. + +```python +def write_audit_cache( + local_path: str, + results: list[CheckResult], + summary: dict[str, int], + level: int, + framework: str, + *, + store: AuditCacheStore | None = None, + cache_key: str | None = None, +) -> None: ... + +def read_audit_cache( + local_path: str, + ttl_seconds: int = 3600, + *, + store: AuditCacheStore | None = None, + cache_key: str | None = None, +) -> dict[str, Any] | None: ... + +def invalidate_audit_cache( + local_path: str, + *, + store: AuditCacheStore | None = None, + cache_key: str | None = None, +) -> None: ... +``` + +**Semantics**: + +* When both `store` and `cache_key` are `None` (backward-compat, external callers who don't know about the store surface yet): the wrapper builds its own default `FilesystemAuditCacheStore(root=/darnit/)` and uses `cache_key = "audit-cache"`. Byte-for-byte legacy path shape. +* When both are provided (audit-driver call site): the wrapper uses them as-is. No introspection of `stores_config`. +* Passing one but not the other is a programmer error; wrapper raises `TypeError` at call time. (Cheap consistency check; keeps the driver honest.) + +**Invariant**: The wrapper's TTL / HEAD-commit / dirty-state staleness logic runs identically in both cases. Only the store instance and key differ. + +## Relationships + +``` +run_sieve_audit(local_path) [tools/audit.py] + -> stores_config (parsed .baseline.toml + framework TOML) + -> stores_bundle (resolve_stores(stores_config, repo_path)) + -> bundle.cache (AuditCacheStore, lazily instantiated) + -> write_audit_cache( + local_path, results, summary, level, framework, + store=bundle.cache, + cache_key=( + "audit-cache" # if stores_config.cache is None + else sha256(abspath(repo_path))[:16] # if configured + ), + ) + -> [inside wrapper] + envelope = build_envelope(results, summary, level, framework, local_path) + store.write(cache_key, envelope) +``` + +Symmetric flow for `read_audit_cache` (called from remediate). Symmetric flow for `invalidate_audit_cache` (called from remediate on user-forced re-audit). diff --git a/specs/035-audit-cache-store-migration/plan.md b/specs/035-audit-cache-store-migration/plan.md new file mode 100644 index 00000000..ada5c088 --- /dev/null +++ b/specs/035-audit-cache-store-migration/plan.md @@ -0,0 +1,165 @@ +# Implementation Plan: Audit-Cache Store Migration + +**Branch**: `035-audit-cache-store-migration` | **Date**: 2026-09-02 | **Spec**: [spec.md](spec.md) + +**Input**: Feature specification from `/specs/035-audit-cache-store-migration/spec.md` + +## Summary + +Route `darnit.core.audit_cache` through feature 033's `AuditCacheStore` Protocol so `[stores.cache]` config in `.baseline.toml` actually redirects the on-disk cache location, and close the wiring gap where `tools/audit.py::run_sieve_audit` still calls `write_audit_cache(local_path, ...)` directly (ignoring `execution_context.stores.cache`). TTL / git-HEAD-commit / working-tree-dirty staleness logic stays in the wrapper (FR-005/006). Cache-key composition is `sha256(abspath(repo_path))[:16]` (FR-003 / clarify Q2). Zero-config default preserves today's path byte-for-byte at `$TMPDIR/darnit//audit-cache.json` (FR-010 / clarify Q1). Invalidation writes an expired envelope so no new `AuditCacheStore.delete()` method is added (FR-009 / clarify Q3). + +Three concrete changes: + +1. **Wrapper (`core/audit_cache.py`)**: `write_audit_cache` / `read_audit_cache` / `invalidate_audit_cache` grow two optional kwargs -- `store: AuditCacheStore | None = None` and `cache_key: str | None = None`. External callers that pass neither get a wrapper-built default `FilesystemAuditCacheStore(root=/darnit/)` with `cache_key = "audit-cache"` (byte-for-byte legacy path). Driver callers pass both explicitly. TTL / HEAD-commit / dirty-state checks live in the wrapper for both paths. Invalidation writes an envelope with `timestamp = "1970-01-01T00:00:00Z"` so the next read misses on TTL. + +2. **`stores/selection.py::resolve_stores`**: change the `cache` default factory's `cache_root` from `repo_path / ".darnit" / "audit-cache"` to `/darnit/`. This is the "aligned to match" clause from clarify Q1: with the default factory now rooted at the per-repo tempdir, the driver's `cache_key = "audit-cache"` picks up the legacy path shape via the existing `/.json` composition in `FilesystemAuditCacheStore._path`. The previous `/.darnit/audit-cache/` default was never called at runtime (feature 033 shipped `bundle.cache` but never wired it in). + +3. **`tools/audit.py::run_sieve_audit`**: replace the direct `write_audit_cache(local_path, ...)` call at line 669 with a store-aware invocation that picks the cache_key based on whether `stores_config.cache` is set: + * `stores_config.cache is None` -> `cache_key = "audit-cache"` (default store already encodes repo identity in its root). + * `stores_config.cache is not None` -> `cache_key = ""` (operator's configured root is shared across repos; per-repo isolation lives in the key). + +All feature 033 constitutional guarantees hold: no new Protocol methods, no new runtime dep, `bundle.cache` stays lazy (only touched when the driver actually needs to write; SC-004 unchanged), and per-store `close()` in `close_all()` still runs only on instantiated stores. + +## Technical Context + +**Language/Version**: Python 3.11 / 3.12 (workspace targets) + +**Primary Dependencies**: stdlib only. `hashlib`, `tempfile`, `subprocess` (all already imported by the wrapper). No new packages. + +**Storage**: Filesystem via the feature 033 `AuditCacheStore` Protocol. Default backend is `FilesystemAuditCacheStore` rooted at the per-repo tempdir; operator can override to `local-fs` or `user-local` per feature 034. + +**Testing**: pytest. Existing surface at `tests/darnit/test_audit_cache.py` anchors backward compat (FR-011); one new integration test at `tests/darnit/test_audit_cache_store_wiring.py` locks the driver-level flow (FR-013 / SC-001 / SC-006). + +**Target Platform**: macOS + Linux fully supported. Windows path handling comes for free via `pathlib` + `tempfile.gettempdir()`; no OS-specific branching added. + +**Project Type**: Library change inside `packages/darnit/`. No new package. + +**Performance Goals**: N/A. One tempfile write per audit, one file read per remediate. Zero cost added to zero-config runs (default store is one lazy construction, identical to feature 033 today). + +**Constraints**: +- No new runtime dependency (FR-012). +- No new `AuditCacheStore` Protocol methods (FR-009 workaround via write-expired). +- All existing `tests/darnit/test_audit_cache.py` MUST pass without modification, except any test that asserts the on-disk directory shape (which MAY be updated to match the new default -- but Q1's byte-for-byte answer means such tests shouldn't exist) OR asserts `write_audit_cache` raises on failure (which is intentionally relaxed by FR-007). Concretely: expect zero test-file diffs in the wrapper's own test module. +- Feature 033's `test_us2_zero_config.py` MUST continue to pass. That test asserts default construction of `bundle.cache`; changing the default `cache_root` inside `resolve_stores` is compatible with its assertions, but re-read it before the tasks phase to confirm. + +**Scale/Scope**: 3 wrapper functions rewritten, 1 call site in `tools/audit.py` updated, 1 default-factory adjustment in `stores/selection.py`. ~150 lines of implementation net, ~200 lines of new/adjusted tests. + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +| Principle | Status | Notes | +|---|---|---| +| I. Plugin Separation | PASS | All changes inside `packages/darnit/` (core + stores + tools). No cross-package imports added; no implementation package touched. | +| II. Conservative-by-Default | PASS with FR-007 note | FR-007 relaxes `write_audit_cache`'s previous "raises on tempfile failure" to feature 033 FR-011's "log warning, continue." Compatible with Principle II because a cache failure never makes an audit **report** incorrect -- it just makes the next remediate slower or forces the sieve loop to re-run. TTL + commit + dirty staleness (the correctness invariants) are preserved verbatim (FR-005, FR-006, SC-004, SC-005). | +| III. TOML-First Architecture | PASS | The whole feature is TOML-driven -- `[stores.cache]` is the surface. | +| IV. Never Guess User Values | N/A | Storage backends don't produce user-judgment values. | +| V. Sieve Pipeline Integrity | N/A | Cache is not a sieve pass; sits above the sieve loop. | + +**Initial gate: PASS with an explicit FR-007 note.** No violations. Re-check after Phase 1 design. + +## Project Structure + +### Documentation (this feature) + +```text +specs/035-audit-cache-store-migration/ +|-- plan.md # this file (/speckit-plan output) +|-- research.md # Phase 0 output +|-- data-model.md # Phase 1 output +|-- quickstart.md # Phase 1 output +|-- contracts/ +| `-- audit-cache-wrapper.md # Phase 1 output +`-- tasks.md # Phase 2 output (/speckit-tasks) +``` + +### Source Code (repository root) + +```text +packages/darnit/src/darnit/ +|-- core/ +| `-- audit_cache.py # REWRITTEN: wrapper over AuditCacheStore. +| # Same public surface plus two optional +| # kwargs; TTL/HEAD/dirty logic preserved. +|-- stores/ +| `-- selection.py # Update default cache_root to +| # /darnit/ so bundle.cache's +| # default backend matches legacy path shape. +`-- tools/ + `-- audit.py # Close wiring gap at line 669: route through + # stores_bundle.cache with cache_key selected + # by whether stores_config.cache is set. + +tests/darnit/ +|-- test_audit_cache.py # EXISTING: MUST pass unchanged +| # (except any test asserting write +| # raises on failure -- FR-007). +`-- test_audit_cache_store_wiring.py # NEW: driver-level integration + # locking FR-013 / SC-001 / SC-006. +``` + +**Structure decision**: single-package extension inside `packages/darnit/`. The wrapper stays in `core/audit_cache.py`; the store class stays in `stores/defaults/cache.py` (feature 033, no change to that file); the default-factory rooting is a one-line change in `stores/selection.py`; the driver-side wiring is a one-block change in `tools/audit.py`. No new files in `core/` or `stores/`. + +## Complexity Tracking + +No constitution violations to justify; table left empty. + +| Violation | Why Needed | Simpler Alternative Rejected Because | +|-----------|------------|-------------------------------------| +| n/a | n/a | n/a | + +## Phase 0: Outline & Research + +Three research items resolvable from source; no NEEDS CLARIFICATION markers survived clarify. + +1. What's the exact wiring gap the driver has to close? Confirm `stores_bundle` is reachable at the cache-write call site in `run_sieve_audit`, and that `stores_config` (the raw config block, needed to distinguish default vs configured) is available at the same scope. +2. What does the pre-feature `test_audit_cache.py` actually assert? Enumerate every test to know which (if any) need adjustment under FR-007 / FR-011. +3. Does changing `resolve_stores`'s default `cache_root` break feature 033's `test_us2_zero_config.py`? If it asserts a specific path shape (`/.darnit/audit-cache/`), that assertion is now stale. + +All three resolvable by grep + read. Consolidated in [research.md](research.md). + +## Phase 1: Design & Contracts + +### Data model + +See [data-model.md](data-model.md). Four entities: + +* **Cache envelope** -- unchanged from pre-feature `write_audit_cache`: `{version, timestamp, commit, commit_dirty, level, framework, results, summary}`. +* **Cache key** -- `sha256(abspath(repo_path))[:16]` when passed by the driver in the configured-store case. Literal `"audit-cache"` in the default-store case (the tempdir root already encodes repo identity). +* **Store-selection rule** (driver-side) -- if `stores_config.cache is None`, driver uses `cache_key = "audit-cache"`; else `cache_key = sha256(abspath(repo_path))[:16]`. +* **Expired envelope** for invalidation -- `{"version": 1, "timestamp": "1970-01-01T00:00:00Z", "commit": null, "commit_dirty": false, "level": 0, "framework": "", "results": [], "summary": {}}` such that the next `read_audit_cache` TTL check misses. + +### Contracts + +See [contracts/audit-cache-wrapper.md](contracts/audit-cache-wrapper.md). Enumerates: + +* Public signatures of `write_audit_cache`, `read_audit_cache`, `invalidate_audit_cache` (backward-compat plus new optional `store` and `cache_key` kwargs). +* Store-selection rule per clarify Q1 (driver picks; wrapper is dumb). +* Cache-key composition per clarify Q2. +* Invalidation via expired-envelope per clarify Q3. +* TTL / commit / dirty staleness enforcement locations (wrapper, not store). +* Error semantics per FR-007 (writes are best-effort; documented change from pre-feature "raises on tempfile failure"). + +### Quickstart + +See [quickstart.md](quickstart.md). Three worked examples: + +1. **CI operator** configures `[stores.cache] backend = "local-fs" root = "$RUNNER_CACHE_DIR/darnit"`, runs an audit twice, sees a cache hit on run 2 because the runner restored `$RUNNER_CACHE_DIR` between jobs. This is the story that was broken pre-feature and works post-feature. +2. **Zero-config user** upgrades darnit; their audit lands its cache at the same `$TMPDIR/darnit//audit-cache.json` as before; nothing changes on disk. +3. **Multi-repo operator** runs audits against two repos with the same shared `[stores.cache] root = "..."`; cache files are `.json` and `.json` under that root, no collision. + +### Agent context update + +CLAUDE.md's `` marker currently points at an older feature's plan. Update to point at this feature's plan at end of Phase 1. + +## Constitution re-check (post-design) + +| Principle | Status | +|---|---| +| I. Plugin Separation | PASS -- three files touched, all inside `packages/darnit/`, no cross-package imports. | +| II. Conservative-by-Default | PASS with FR-007 note -- documented in Constitution Check above and repeated in the contracts doc. | +| III. TOML-First Architecture | PASS | +| IV. Never Guess User Values | N/A | +| V. Sieve Pipeline Integrity | N/A | + +**Final gate: PASS.** Ready for `/speckit-tasks`. diff --git a/specs/035-audit-cache-store-migration/quickstart.md b/specs/035-audit-cache-store-migration/quickstart.md new file mode 100644 index 00000000..511145e8 --- /dev/null +++ b/specs/035-audit-cache-store-migration/quickstart.md @@ -0,0 +1,107 @@ +# Quickstart: `[stores.cache]` Actually Redirects the Audit Cache + +**Feature**: 035-audit-cache-store-migration + +Three worked examples an operator can walk through to convince themselves the migration behaves. Each example assumes darnit is installed (`uv tool install darnit` or equivalent) and there's a git repo at `~/src/my-project` to audit against. + +## Example 1: CI operator, `local-fs` backend under `$RUNNER_CACHE_DIR` + +**Scenario**: a GitHub Actions job that audits the repo on every PR. The runner caches the `$RUNNER_CACHE_DIR/darnit` directory between jobs so a second audit of the same PR commit hits the cache and skips the sieve loop. + +**Config** (`.baseline.toml` at repo root): + +```toml +[stores.cache] +backend = "local-fs" +root = "$RUNNER_CACHE_DIR/darnit" +``` + +**Run 1** (fresh cache): + +```bash +$ export RUNNER_CACHE_DIR=/tmp/ci-cache +$ mkdir -p "$RUNNER_CACHE_DIR" +$ darnit audit ~/src/my-project --level 1 +[...] audit runs the sieve loop, produces results [...] +INFO wrote cache (local-fs): /tmp/ci-cache/darnit/.json +``` + +The cache file lands under `$RUNNER_CACHE_DIR/darnit/`, NOT `$TMPDIR/darnit/`. Confirm: + +```bash +$ ls /tmp/ci-cache/darnit/ +5a4c8e9f2b1d3706.json # is sha256(abspath(~/src/my-project))[:16] + +$ find /tmp/darnit -name "audit-cache.json" 2>/dev/null +# (no output -- no fallback cache in system tempdir) +``` + +**Run 2** (cache hit, no git changes between runs): + +```bash +$ darnit audit ~/src/my-project --level 1 +[...] audit skips the sieve loop, reuses cached results [...] +``` + +The audit driver reads `/tmp/ci-cache/darnit/5a4c8e9f2b1d3706.json`, checks TTL + HEAD + dirty-state (all match), and returns the cached envelope directly. This is the story that was broken pre-feature-035 -- the `[stores.cache]` block used to do nothing. + +## Example 2: Zero-config user, upgrade path + +**Scenario**: a developer running darnit locally who never touched `.baseline.toml`. They upgrade to a darnit release that includes this feature and expect nothing to change on disk. + +**Config**: none. No `.baseline.toml`, or a `.baseline.toml` with no `[stores.*]` block. + +**Run**: + +```bash +$ darnit audit ~/src/my-project --level 1 +[...] audit runs, produces results [...] +``` + +Cache location: + +```bash +$ REPO_HASH=$(python -c "import hashlib, os; print(hashlib.sha256(os.path.abspath(os.path.expanduser('~/src/my-project')).encode()).hexdigest()[:16])") +$ ls "${TMPDIR:-/tmp}/darnit/$REPO_HASH/" +audit-cache.json +``` + +Byte-for-byte identical to where the pre-feature-035 darnit put it. No release-notes surprise for zero-config users; the on-disk path is unchanged. + +## Example 3: Multi-repo operator, shared `local-fs` root + +**Scenario**: a maintainer runs audits against several repos from the same machine, all sharing one `[stores.cache] root`. Verifies per-repo isolation. + +**Config** (in each repo's `.baseline.toml`): + +```toml +[stores.cache] +backend = "local-fs" +root = "~/.cache/darnit-shared" +``` + +**Runs**: + +```bash +$ darnit audit ~/src/repo-a --level 1 +$ darnit audit ~/src/repo-b --level 1 +$ ls ~/.cache/darnit-shared/ +5a4c8e9f2b1d3706.json # sha256(abspath(~/src/repo-a))[:16] +9c7f2a4b8e0d1583.json # sha256(abspath(~/src/repo-b))[:16] +``` + +Two files, one per repo. Confirms SC-006: two different repos under a shared root do not overwrite each other. If we had used `cache_key = "audit-cache"` in the configured-store case (as we do in the default-store case), both repos would collide on a single file -- that's why the wrapper switches to a per-repo cache_key when the operator has configured a root. + +## Bonus: invalidation + +If you want to force a fresh audit without changing any code or committing anything: + +```bash +$ python -c "from darnit.core.audit_cache import invalidate_audit_cache; invalidate_audit_cache('$HOME/src/my-project')" +$ darnit audit ~/src/my-project --level 1 +[...] audit runs the sieve loop (cache miss, no reuse) [...] +``` + +Behind the scenes, `invalidate_audit_cache` wrote an envelope with `timestamp = "1970-01-01T00:00:00Z"` to the cache location; the next `read_audit_cache` misses on the TTL check. + +The file itself remains on disk until the next audit writes over it. That's intentional: the `AuditCacheStore` Protocol has no `delete(key)` method (adding one would ripple through every backend implementation), and the cache path is operator-invisible tempdir in the default case anyway. diff --git a/specs/035-audit-cache-store-migration/research.md b/specs/035-audit-cache-store-migration/research.md new file mode 100644 index 00000000..d75587e5 --- /dev/null +++ b/specs/035-audit-cache-store-migration/research.md @@ -0,0 +1,79 @@ +# Phase 0: Research -- Audit-Cache Store Migration + +**Feature**: 035-audit-cache-store-migration | **Date**: 2026-09-02 + +All three research items resolved by direct source inspection. No NEEDS CLARIFICATION markers survived the /speckit-clarify pass. + +## R-001: Wiring gap in `run_sieve_audit` + +**Question**: Confirm `stores_bundle` and `stores_config` are both reachable at the cache-write call site in `packages/darnit/src/darnit/tools/audit.py`. + +**Decision**: Both are reachable. The driver already computes them at `audit.py:562-564`: + +```python +stores_config = _load_merged_stores(local_path, resolved_fw) +stores_bundle = resolve_stores(stores_config, repo_path=Path(local_path)) +execution_context.stores = stores_bundle +``` + +...and the direct cache-write call sits at `audit.py:667-671`, well inside the scope where both bindings are still live. The wiring gap is a purely lexical omission: the current code imports `write_audit_cache` from `core.audit_cache` and calls it with `local_path` only, ignoring the bundle that's already sitting one scope up. + +**Rationale**: No plumbing work needed. Reuse the two bindings that feature 033 already set up. `stores_config.cache` (may be `None`) is the signal for "operator has configured a cache backend"; `stores_bundle.cache` (never `None`; lazily instantiated) is the store instance we call `.read` / `.write` on. + +**Alternatives considered**: Threading a fifth parameter into `write_audit_cache` from the outside (e.g., `stores_config`). Rejected because the wrapper doesn't need to know the config, only the store instance and the key. + +## R-002: Existing test surface at `tests/darnit/core/test_audit_cache.py` + +**Question**: Enumerate every existing assertion so we know which tests need adjustment vs which pass unchanged. + +**Decision**: 30+ tests across 10 test classes. Under Q1's "byte-for-byte identical default path" answer, the vast majority pass unmodified. Two tests need explicit adjustment: + +1. **`TestAtomicWrite::test_no_partial_file_on_error`** (line 280-293) asserts `write_audit_cache` raises `OSError` when `json.dump` fails. FR-007 relaxes this to "log warning, continue." Adjustment: replace `with pytest.raises(OSError):` with a call that must NOT raise, and keep the "no partial file left behind" assertion. The "no partial file" invariant is now enforced by `FilesystemAuditCacheStore.write`'s tempfile-then-rename + tempfile-cleanup logic. + +2. **`TestInvalidateCache::test_invalidate_existing`** (line 256-262) asserts `cache_path.exists()` is False after `invalidate_audit_cache`. Under Q3's write-expired-envelope semantics, the file still exists but its timestamp is `1970-01-01T00:00:00Z`. Adjustment: replace with `assert read_audit_cache(str(temp_git_repo)) is None` -- the observable behavior ("the next read misses") is what actually matters to callers. + +All other tests use `_get_cache_dir(...)` to compute the expected file path; that helper remains and continues to return `$TMPDIR/darnit//`, so those tests remain valid under Q1's byte-for-byte answer. + +**Rationale**: The spec's FR-011 already documents "test that asserts write raises on failure" as an intentional exception. R-002 identifies a second exception (`test_invalidate_existing`) that FR-011 doesn't currently name. Recommendation: extend FR-011's exception list at tasks-phase time so the task-list reader knows to touch both tests. (Not treating this as a spec re-clarify -- Q3's answer explicitly said "The cache file remains on disk until the next successful write overwrites it," which is the same behavior change surfacing as a test adjustment.) + +**Alternatives considered**: Instead of writing an expired envelope, have `invalidate_audit_cache` seek out the underlying store instance and call a hypothetical `.delete(key)` method. Rejected because Q3 explicitly chose the write-expired approach to avoid adding a Protocol method. + +## R-003: Feature 033's `test_us2_zero_config.py::test_filesystem_defaults_use_canonical_darnit_paths` + +**Question**: Does changing `resolve_stores`'s default `cache_root` break this test? + +**Decision**: YES, it breaks one assertion. Line 76 asserts: + +```python +assert bundle.cache._root == tmp_path / ".darnit" / "audit-cache" +``` + +Post-migration, the default `cache_root` becomes `/darnit/`, NOT `/.darnit/audit-cache`. Adjustment: the assertion becomes something like: + +```python +import hashlib +import tempfile +from pathlib import Path + +expected_hash = hashlib.sha256(str(tmp_path.resolve()).encode()).hexdigest()[:16] +expected_root = Path(tempfile.gettempdir()) / "darnit" / expected_hash +assert bundle.cache._root == expected_root +``` + +The other two assertions in the same test (`bundle.attestation._root == tmp_path / ".darnit" / "attestations"` and `bundle.report._root == tmp_path / ".darnit" / "reports"`) are unchanged; the migration only rebases the cache default. + +**Rationale**: This is exactly what spec SC-008 covers: "If a zero-config default location change is chosen (per Clarifications), that test is updated in this feature's PR with a documented reason." Q1 chose "preserve today's default at $TMPDIR/darnit/" AND "FilesystemAuditCacheStore default aligned to match this scheme," so this test adjustment is required and pre-authorized. + +The `test_none_config_yields_filesystem_defaults_for_all_four` and `test_no_plugin_backend_constructed_under_zero_config` tests in the same file continue to pass unchanged -- they only assert the type/lazy behavior, not the specific root path. + +**Alternatives considered**: Keep the `/.darnit/audit-cache` default in `resolve_stores` and have the wrapper skip `bundle.cache` for zero-config runs (constructing its own tempdir store instead). Rejected because it forks the code (wrapper decides "use bundle vs build own" on a config-driven condition), duplicates path composition logic between the wrapper and the store, and makes future audit-driver features that touch `bundle.cache` inconsistent with what `write_audit_cache` does. + +## Summary + +| ID | Item | Impact | Resolution | +|----|------|--------|------------| +| R-001 | Wiring gap in `run_sieve_audit` | 3-line change at `audit.py:667` | Reuse `stores_bundle.cache` + pick key by `stores_config.cache is None` | +| R-002 | `test_audit_cache.py` adjustments | 2 tests (out of 30+) | `test_no_partial_file_on_error`: no raise. `test_invalidate_existing`: check read result, not file existence. | +| R-003 | `test_us2_zero_config.py` adjustment | 1 assertion (line 76) | Compute expected root as `/darnit/` | + +All items resolvable in-implementation; no unknowns block Phase 1. diff --git a/specs/035-audit-cache-store-migration/spec.md b/specs/035-audit-cache-store-migration/spec.md new file mode 100644 index 00000000..f67c2b68 --- /dev/null +++ b/specs/035-audit-cache-store-migration/spec.md @@ -0,0 +1,181 @@ +# Feature Specification: Audit-Cache Store Migration + +**Feature Branch**: `035-audit-cache-store-migration` + +**Created**: 2026-09-02 + +**Status**: Draft + +**Input**: User description: "Migrate darnit.core.audit_cache to route through the feature-033 AuditCacheStore protocol. Today write_audit_cache/read_audit_cache use their own path scheme ($TMPDIR/darnit//audit-cache.json) that ignores [stores.cache] TOML config. The migration should make [stores.cache] backend/root override the path, preserve TTL and git-commit + dirty-state staleness logic in the wrapper, and pick a sensible default location. Also close the wiring gap so tools/audit.py routes cache writes through bundle.cache rather than calling write_audit_cache directly." + +## Clarifications + +### Session 2026-09-02 + +- Q: Where does the cache land when the operator has not configured `[stores.cache]`? → A: Preserve today's default -- `$TMPDIR/darnit//audit-cache.json`. Zero-config path unchanged; upgrade is transparent. Feature 033's `FilesystemAuditCacheStore` default is aligned to match this scheme (its previous `/.darnit/audit-cache/` default was an unused artifact -- `resolve_stores` still returned a `FilesystemAuditCacheStore` instance but the audit driver never called it). The `[stores.cache] backend = "local-fs" root = ".darnit/audit-cache"` config still works for operators who prefer the in-repo layout. +- Q: How is the `cache_key` composed for `bundle.cache.read/write` calls? → A: `sha256(abspath(repo_path))[:16]`. Identical to the hash the pre-feature `_get_cache_dir` used for the directory prefix, so per-repo isolation is preserved verbatim under a shared `root`. No framework/level in the key; those are embedded in the envelope, matching today's overwrite-on-mismatch behavior. Filename on disk is `.json` per feature 033's `FilesystemAuditCacheStore` layout. +- Q: How does `invalidate_audit_cache` clear the entry, given `AuditCacheStore` has no `delete` method? → A: Write an expired envelope (timestamp `1970-01-01T00:00:00Z`). The next `read_audit_cache` sees the ancient timestamp and returns miss via the existing TTL check. Zero Protocol change; backwards-compatible with existing store implementations and any future ones (Postgres, S3, etc.). The cache file remains on disk until the next successful write overwrites it -- acceptable because the current cache path is operator-invisible tempdir anyway. + +## Context + +Feature 033 (PR #396) landed the `AuditCacheStore` Protocol and the `FilesystemAuditCacheStore` default. Feature 034 (PR #412) added the `local-fs` and `user-local` backends. Both features gave operators the vocabulary to configure `[stores.cache]` in `.baseline.toml`. The catch: **nothing wires that configuration into the actual audit-cache read/write path today.** + +The truth on disk (verified by inspection of `packages/darnit/src/darnit/core/audit_cache.py` and `packages/darnit/src/darnit/tools/audit.py`): + +- `tools/audit.py::run_sieve_audit` calls `core.audit_cache.write_audit_cache(local_path, ...)` directly after the sieve loop. +- `core/audit_cache.py::_get_cache_dir(local_path)` computes `$TMPDIR/darnit//audit-cache.json`. No store, no `[stores.cache]` awareness. +- `bundle.cache` (the `AuditCacheStore` from feature 033's `resolve_stores`) IS instantiated per audit run but never read from or written to by the driver. +- An operator who writes `[stores.cache] backend = "local-fs" root = "/mnt/x"` in `.baseline.toml` today gets ZERO effect on where their cache goes. + +Feature 033 explicitly deferred this migration (T026 in its tasks.md); this feature closes the loop. The complexity is not the plumbing (small) but the default-location question: today's default is system tempdir per-repo-hash; feature 033's `FilesystemAuditCacheStore` expects `/.darnit/audit-cache/`; those are different places and the choice is user-visible. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Operator configures `[stores.cache]` and the cache actually moves (Priority: P1) + +An operator running a CI pipeline sets: + +```toml +[stores.cache] +backend = "local-fs" +root = "$RUNNER_CACHE_DIR/darnit" +``` + +...and expects the audit-cache file to land under `$RUNNER_CACHE_DIR/darnit/`, not in a hashed system tempdir. Second-run cache hits work because the runner restored `$RUNNER_CACHE_DIR` between jobs. + +**Why this priority**: This is the concrete gap operators are hitting today. The vocabulary shipped in features 033 and 034 doesn't do anything for cache until this lands. Highest user-visible impact. + +**Independent Test**: Configure `[stores.cache] backend = "local-fs" root = "/cache"`, run an audit twice against the same commit + clean tree. Verify: (a) `/cache/.json` exists after the first run; (b) the second run's cache read hits (audit skips the sieve loop); (c) no cache file lands under `$TMPDIR/darnit/`. + +**Acceptance Scenarios**: + +1. **Given** `[stores.cache] backend = "local-fs" root = "/tmp/x/cache"` and no prior cache, **When** the audit runs, **Then** the cache file lands under `/tmp/x/cache/` and NOT under `$TMPDIR/darnit/`. +2. **Given** the same config with a fresh cache miss on run 1, **When** run 2 executes against the same commit + working-tree state, **Then** the cache read HITS and the sieve loop is skipped. +3. **Given** the same config but git HEAD changed between runs, **When** run 2 executes, **Then** the cache read MISSES (staleness detection preserved) and the sieve loop runs. +4. **Given** `[stores.cache] backend = "user-local"`, **When** the audit runs on Linux with default XDG paths, **Then** the cache file lands under `~/.cache/darnit/audit-cache/`. + +--- + +### User Story 2 - Zero-config default is documented and predictable (Priority: P1) + +An operator who never touches `.baseline.toml` gets a deterministic cache location. Cross-audit-run behavior (hit/miss) is unchanged from the current implementation. If the default location changes as part of this migration, the release notes call it out. + +**Why this priority**: Even users who never configure `[stores.cache]` are affected because the default path is a decision this feature MUST make. Getting it wrong (surprise-moving cache without documentation) breaks user trust. + +**Independent Test**: Run an audit with no `[stores.*]` block. Locate the on-disk cache file. Confirm the path matches the documented default AND is stable (same path on repeat runs). + +**Acceptance Scenarios**: + +1. **Given** no `[stores.*]` block, **When** the audit runs, **Then** the cache file lands at the documented default location. +2. **Given** two successive audits with no config change, **When** they both run, **Then** the cache location is identical (no per-audit variability). +3. **Given** an operator upgrades from a pre-feature-035 darnit to a post-feature-035 darnit and runs the same audit, **When** the audit runs, **Then** the operator can find the release-notes entry naming the default cache location (whether unchanged or moved). + +--- + +### User Story 3 - TTL and staleness detection preserved (Priority: P1) + +Existing cache-invalidation semantics MUST work through the store: 3600-second TTL, git HEAD commit hash change invalidates, and working-tree dirty state change invalidates. These live in the wrapper (`core/audit_cache.py`), NOT in the store; the store is a plain read-through/write-through KV. + +**Why this priority**: A cache with correctness gaps (stale hits) is worse than no cache. The existing invalidation logic is trusted; this feature MUST preserve it byte-for-byte. + +**Independent Test**: Reuse the existing test surface at `tests/darnit/test_audit_cache.py` (or equivalent). All existing tests MUST pass unchanged. Add one new test that exercises invalidation over a `local-fs` cache backend to prove the wrapper still gates the store's raw reads. + +**Acceptance Scenarios**: + +1. **Given** a cache write followed by a read within 3600 seconds, no git changes, **When** the read runs, **Then** it HITS. +2. **Given** a cache write, then `git commit --allow-empty` (HEAD changes), **When** a subsequent read runs, **Then** it MISSES. +3. **Given** a cache write with clean tree, then dirtying the tree, **When** a subsequent read runs, **Then** it MISSES. +4. **Given** a cache write more than 3600 seconds ago, **When** a read runs, **Then** it MISSES. +5. Same acceptance scenarios (1)-(4) hold when the store is `local-fs` with an explicit `root`, not just the default backend. + +--- + +### User Story 4 - Public API of `write_audit_cache` / `read_audit_cache` preserved (Priority: P2) + +Any code that today calls `from darnit.core.audit_cache import write_audit_cache, read_audit_cache, invalidate_audit_cache` continues to work with the same call signatures. The functions become thin wrappers over `bundle.cache.read` / `.write` but their public surface is unchanged. + +**Why this priority**: Backward compatibility. External consumers (tests, other darnit subsystems, third-party tools) may import these names. + +**Independent Test**: `grep -rn "from darnit.core.audit_cache import"` before and after the migration produces the same import list. Every consumer's tests continue to pass. + +**Acceptance Scenarios**: + +1. **Given** existing test `tests/darnit/test_audit_cache.py::*`, **When** it runs against the migrated implementation, **Then** it passes without modification. +2. **Given** the darnit-baseline package's use of the cache (if any), **When** it runs, **Then** its behavior is unchanged. + +--- + +### Edge Cases + +- **First-run miss**: `bundle.cache.read` returns `None` for a nonexistent key. `read_audit_cache` translates that to "cache miss" cleanly (already the pre-feature behavior). +- **Concurrent audits of the same repo** with the same configured `root`: two processes racing on the same cache_key. Cache is best-effort per feature 033 FR-011; the tempfile-then-rename in `FilesystemAuditCacheStore` handles this without corruption, but the "loser" race silently overwrites the "winner". Acceptable per the best-effort contract. +- **`bundle.cache` swallows write errors** (FR-011). Legacy `write_audit_cache` did not — it raised on tempfile / rename failures. The migration MUST reconcile: does the wrapper adopt best-effort semantics, or does it re-surface errors from the store? Chosen behavior: wrapper adopts best-effort. Existing writes that raised now log-and-continue. Existing callers that caught the exception now see silent success (with a warning log). +- **`stores.cache` selection fails at `resolve_stores` time** (`StoreNotInstalled`, `StoreProtocolMismatch`): the audit tool already dies at startup before the sieve loop begins. No change here; feature 033's fail-fast contract holds. +- **Legacy per-repo-hash cache dir left over**: if this feature changes the default location, old `$TMPDIR/darnit//` directories are orphaned. Cleanup is out of scope; document that operators can remove them safely. +- **Cache-cache-key collision across repos**: today's key is a fixed filename `audit-cache.json` under a per-repo-hash directory. Post-migration, if all repos share one `root` (US1 CI case) the store needs a per-repo cache_key so they don't overwrite each other. The wrapper MUST compose a cache_key that includes the repo identity. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: The audit driver (`tools/audit.py::run_sieve_audit`) MUST route cache writes through `bundle.cache.write(cache_key, envelope)` -- NOT through a direct call to `core.audit_cache.write_audit_cache(local_path, ...)`. +- **FR-002**: `core.audit_cache.write_audit_cache` and `core.audit_cache.read_audit_cache` MUST become thin wrappers over the store. They accept a `store: AuditCacheStore` parameter (with backward-compat default from a helper that resolves it lazily if not passed). Their public call signatures do NOT otherwise change. +- **FR-003**: The cache_key composed by the wrapper MUST be `sha256(abspath(repo_path))[:16]` -- identical to the hash the pre-feature `_get_cache_dir` used for the tempdir prefix. This guarantees per-repo isolation under a shared `root` and preserves stability across runs on the same repo path. Framework and level are NOT included in the key; they remain in the envelope, matching today's overwrite-on-mismatch behavior. +- **FR-004**: When an operator sets `[stores.cache] backend = "local-fs" root = "..."`, cache writes MUST land under that root and reads MUST see them. Verified end-to-end via an audit, not just at the store layer. +- **FR-005**: TTL enforcement (default 3600 seconds) MUST live in the wrapper. The store MUST NOT know about TTL. Existing TTL semantics MUST be preserved verbatim. +- **FR-006**: Git-HEAD-commit and working-tree-dirty-state staleness detection MUST live in the wrapper. Existing detection logic MUST be preserved verbatim. +- **FR-007**: `bundle.cache.write` failures MUST NOT propagate to the audit-run's exit code. The legacy `write_audit_cache` behavior of raising on tempfile failure is REPLACED with feature 033 FR-011's best-effort semantics: log a warning, continue. This is a documented behavior change. +- **FR-008**: `bundle.cache.read` returning `None` MUST be treated as cache miss (audit runs fresh). No change from today. +- **FR-009**: The public function `invalidate_audit_cache(local_path)` MUST continue to work as `bundle.cache.write(cache_key, )`. The expired envelope carries `timestamp = "1970-01-01T00:00:00Z"` so the next `read_audit_cache` misses via the existing TTL check. No `AuditCacheStore.delete()` method is added; the Protocol surface is unchanged. +- **FR-010**: The default cache location when no `[stores.cache]` block is configured MUST be `$TMPDIR/darnit//audit-cache.json` -- byte-for-byte identical to today's path. Release notes may mention that the wrapper is now store-routed under the hood, but no on-disk behavior change lands for zero-config users. `FilesystemAuditCacheStore`'s in-code default constructor arg is updated in this feature's PR to match; the previous `/.darnit/audit-cache/` value was an unused-in-practice artifact of feature 033's `resolve_stores`. +- **FR-011**: All existing tests under `tests/darnit/core/test_audit_cache.py` (and the equivalent test surface for `write_audit_cache` / `read_audit_cache`) MUST pass without modification, except tests that fall into one of these three intentionally-relaxed categories: + 1. Tests asserting the on-disk path shape MAY be updated to match the new default (if the default location changes). + 2. Tests asserting `write_audit_cache` raises on tempfile / rename failure MUST be updated to expect no-raise (intentionally relaxed by FR-007). + 3. Tests asserting `invalidate_audit_cache` removes the on-disk cache file MUST be updated to check `read_audit_cache(...) is None` instead (intentionally relaxed by FR-009's write-expired-envelope semantics; the file remains on disk with an expired timestamp). +- **FR-012**: The wrapper MUST NOT introduce any new runtime dependency. It uses only the existing `darnit.stores` surface + `subprocess` (already used) for git introspection. +- **FR-013**: A test MUST exercise the end-to-end `[stores.cache] backend = "local-fs" root = "..."` flow via a real audit invocation (or a driver-level fixture), not just the store layer in isolation. This locks the wiring gap fix. +- **FR-014**: The `bundle.cache` property from `resolve_stores` is lazy-instantiated (per feature 033 SC-004). An audit that never touches the cache -- e.g., one with `stop_on_llm=True` and a first-time run that never gets to `write_audit_cache` -- MUST NOT construct the cache backend. The migration preserves this invariant. + +### Key Entities + +- **`bundle.cache`**: the `AuditCacheStore` instance the wrapper reads/writes through. Resolved once per audit run from `[stores.cache]` config; `FilesystemAuditCacheStore` when unset. +- **`cache_key`**: string uniquely identifying (repo, framework, level) so two different audit shapes against the same repo, and two different repos against the same `root`, don't collide. Composition rule established in this feature (see Clarifications). +- **Cache envelope**: existing JSON shape at `core/audit_cache.py` -- version, timestamp, commit, commit_dirty, level, framework, results, summary. Unchanged by this feature. +- **Default cache root**: the path where cache lands when no `[stores.cache]` is set. Subject to a clarify question. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: An operator configuring `[stores.cache] backend = "local-fs" root = ""` in `.baseline.toml`, running an audit twice back-to-back with no code change between runs, sees a cache hit on run 2 AND the cache file located under ``. Verified by a driver-level integration test. +- **SC-002**: An operator running with no `[stores.*]` block sees the cache land at the documented default location. Verified by a test that captures the actual write path. +- **SC-003**: All existing `tests/darnit/test_audit_cache.py` tests pass on the migrated implementation, except the one test (if any) documented in the release notes as being intentionally relaxed by FR-007's best-effort semantics. +- **SC-004**: A repository whose git HEAD changes between two consecutive audits MUST see a cache miss on the second audit. Verified by an end-to-end test that mutates HEAD between reads. +- **SC-005**: A repository whose working tree becomes dirty between two audits MUST see a cache miss on the second audit. +- **SC-006**: Two different repos configured against the same `[stores.cache] root = ""` MUST NOT overwrite each other's cache files. Verified by a test that runs two audits with distinct `repo_path` values against a shared root. +- **SC-007**: A single `cache.write` failure (e.g., disk full at the target root) MUST NOT propagate to the audit's exit code. The audit completes successfully; a warning is logged. Verified by fault-injection unit test. +- **SC-008**: No test module named `tests/darnit/stores/test_us2_zero_config.py` (feature 033's zero-config witness) fails because of this migration. If a zero-config default location change is chosen (per Clarifications), that test is updated in this feature's PR with a documented reason. +- **SC-009**: The migration lands within the file-scope boundary: `packages/darnit/src/darnit/core/audit_cache.py`, `packages/darnit/src/darnit/tools/audit.py`, `packages/darnit/src/darnit/stores/defaults/cache.py` (if the default location change requires updating `FilesystemAuditCacheStore`'s init default), `tests/darnit/test_audit_cache.py`, and one new integration test file. No plugin-package or implementation-package modifications. + +## Assumptions + +- The store instance for the current audit is available to the wrapper via the `execution_context.stores` bundle already threaded through `tools/audit.py` in feature 033 T023. The wrapper does NOT need to re-run `resolve_stores` itself. +- `bundle.cache.write` swallowing failures per the Protocol is acceptable operator behavior. Loud failure was a nice-to-have but not a documented contract of the pre-feature `write_audit_cache`. +- The `AuditCacheStore` Protocol does NOT currently expose a `delete(key)` method. Clarify Q3 declined adding one; invalidation uses a write-expired-envelope workaround. The Protocol surface remains unchanged. +- Feature 034 (#412) is already merged before this feature ships. If it isn't, this feature's PR body notes the dependency. +- The default cache location's dependency on `repo_path` (per-repo isolation) is a core requirement, not a nice-to-have. Operators should be able to run darnit on multiple repos from the same machine without cache poisoning. + +## Dependencies + +- Feature 033 (PR #396, merged): the `AuditCacheStore` Protocol, `FilesystemAuditCacheStore`, `_StoreBundle`, `resolve_stores`, `execution_context.stores`. +- Feature 034 (PR #412, review pending): the `local-fs` and `user-local` backends. This feature's US1 depends on `local-fs` being routable end-to-end. +- No external service dependencies. + +## Out of Scope + +- **Audit-cache format change**. The JSON envelope shape (version, timestamp, commit, commit_dirty, level, framework, results, summary) is unchanged. Adding fields is a separate spec. +- **Cache retention / rotation / eviction policy** beyond the existing TTL. Operators manage cleanup of their configured `root`. +- **A new `AuditCacheStore.delete(key)` method**. Clarify resolved: invalidation uses a write-expired-envelope workaround (spec Q3), so the Protocol stays unchanged. +- **Migration of old cache files** from the current `$TMPDIR/darnit//` layout to whatever the new default is. Operators can `rm -rf` those manually. +- **Wiring of `bundle.attestation` and `bundle.report`** into the audit driver. Those are separate follow-ups (from the same audit driver but different call sites). +- **Cross-process cache locking**. Two darnit processes writing to the same cache_key at the same time is the store's problem to solve via tempfile-then-rename; no new locking layer. diff --git a/specs/035-audit-cache-store-migration/tasks.md b/specs/035-audit-cache-store-migration/tasks.md new file mode 100644 index 00000000..0a57316a --- /dev/null +++ b/specs/035-audit-cache-store-migration/tasks.md @@ -0,0 +1,213 @@ +--- + +description: "Task breakdown for feature 035 (audit-cache store migration)" +--- + +# Tasks: Audit-Cache Store Migration + +**Input**: Design documents from `/specs/035-audit-cache-store-migration/` + +**Prerequisites**: [plan.md](plan.md), [spec.md](spec.md), [research.md](research.md), [data-model.md](data-model.md), [contracts/audit-cache-wrapper.md](contracts/audit-cache-wrapper.md), [quickstart.md](quickstart.md) + +**Tests**: Tests ARE included. FR-011 requires existing surface to keep passing; FR-013 requires a new driver-level integration test; SC-001/SC-006 are test-verified. Test tasks are explicit below. + +**Organization**: Tasks are grouped by user story (US1..US4) so each story is independently testable. Foundational (Phase 2) wraps the three code touches that unblock every user story. + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: Can run in parallel (different files, no dependencies on incomplete tasks) +- **[Story]**: Which user story this task belongs to (US1..US4) +- Include exact file paths in descriptions + +## Path Conventions + +- Product code: `packages/darnit/src/darnit/` +- Tests: `tests/darnit/` + +--- + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Verify preconditions. No new dependencies, no scaffolding. + +- [X] T001 Verify feature 033 (`bundle.cache`, `FilesystemAuditCacheStore`, `resolve_stores`) and feature 034 (`LocalFsAuditCacheStore`, `UserLocalAuditCacheStore`) are importable from a clean `uv sync`: `uv run python -c "from darnit.stores.selection import resolve_stores; from darnit.stores.defaults.local_fs import LocalFsAuditCacheStore; print('ok')"`. If either import fails, halt and rebase. + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: The three code touches that make every user story work. All three land together. + +**CRITICAL**: No user-story test tasks can pass until this phase is complete. + +- [X] T002 Update `packages/darnit/src/darnit/stores/selection.py`: change the `cache` default_factory in `resolve_stores` so `cache_root` defaults to `Path(tempfile.gettempdir()) / "darnit" / hashlib.sha256(str(repo_path.resolve()).encode()).hexdigest()[:16]` (instead of `repo_path / ".darnit" / "audit-cache"`). Add `import hashlib` and `import tempfile` at the top of the file. Leave the attestation/report/project defaults unchanged (they still use `/.darnit/{attestations,reports}` and `/.project/`). + +- [X] T003 Rewrite `packages/darnit/src/darnit/core/audit_cache.py`: keep the module's public docstring, `CACHE_FILENAME`, `CACHE_VERSION`, `_get_head_commit`, `_is_working_tree_dirty`, and `_get_cache_dir` (the last is kept as an internal helper for the default-store backward-compat path and for existing test fixtures). Introduce a module-level `_EXPIRED_ENVELOPE = {"version": 1, "timestamp": "1970-01-01T00:00:00Z", "commit": None, "commit_dirty": False, "level": 0, "framework": "", "results": [], "summary": {}}`. Rewrite `write_audit_cache(local_path, results, summary, level, framework, *, store=None, cache_key=None)`, `read_audit_cache(local_path, ttl_seconds=3600, *, store=None, cache_key=None)`, and `invalidate_audit_cache(local_path, *, store=None, cache_key=None)` per [contracts/audit-cache-wrapper.md](contracts/audit-cache-wrapper.md). When both `store` and `cache_key` are `None`, wrapper builds `FilesystemAuditCacheStore(root=_get_cache_dir(local_path))` and uses `cache_key = "audit-cache"` (byte-for-byte legacy path). If exactly one of the two is `None`, raise `TypeError("store and cache_key must be passed together")`. TTL / null-commit / HEAD-commit / dirty-state staleness checks stay in `read_audit_cache` (unchanged logic). `write_audit_cache` MUST NOT raise on any `OSError` from the store (FR-007); wrap the `store.write` call in try/except and log at warning level. + +- [X] T004 Close the wiring gap in `packages/darnit/src/darnit/tools/audit.py::run_sieve_audit` (around line 667). Replace the direct `write_audit_cache(local_path, all_results, summary, level, resolved_fw or "")` with a store-routed call: compute `cache_key = "audit-cache" if (stores_config is None or stores_config.cache is None) else hashlib.sha256(str(Path(local_path).resolve()).encode()).hexdigest()[:16]`, then call `write_audit_cache(local_path, all_results, summary, level, resolved_fw or "", store=stores_bundle.cache, cache_key=cache_key)`. Import `hashlib` at the top of the file if not already present. Keep the enclosing `try/except Exception` (defensive belt-and-braces) but note the wrapper now swallows internally per FR-007. + +**Checkpoint**: Foundation ready. All existing `tests/darnit/core/test_audit_cache.py` cases that don't touch the two behavior-change assertions (see US3 tasks) should now pass without modification. Run `uv run pytest tests/darnit/core/test_audit_cache.py -v` to sanity-check before proceeding. + +--- + +## Phase 3: User Story 1 - Operator configures `[stores.cache]` (Priority: P1) MVP + +**Goal**: Operator writes `[stores.cache] backend = "local-fs" root = "..."` in `.baseline.toml`, runs an audit twice, sees cache-file-under-root AND a cache hit on run 2. + +**Independent Test**: Configure `[stores.cache] backend = "local-fs" root = "/cache"` in a temp repo, run the audit driver twice, assert `/cache/.json` exists and second run reads it. + +### Tests for User Story 1 + +> Write these tests FIRST; they should FAIL before T004 lands and PASS after. + +- [X] T005 [US1] Create `tests/darnit/test_audit_cache_store_wiring.py` with a `TestLocalFsBackendRouting` class. Add `test_config_moves_cache_to_configured_root`: (a) set up a temp git repo via the existing `temp_git_repo` fixture, (b) write a `.baseline.toml` at the repo root with `[stores.cache] backend = "local-fs"` and `root = "/cache-out"`, (c) call `run_sieve_audit` with a mocked orchestrator (following the pattern from `TestRunSieveAuditCacheIntegration::test_run_sieve_audit_writes_cache` in `tests/darnit/core/test_audit_cache.py`), (d) assert a `.json` file exists under `/cache-out/` with name `.json`, (e) assert NO file exists under `/darnit/`. + +- [X] T006 [US1] Add `test_second_run_reads_cache_via_configured_store` to the same class in `tests/darnit/test_audit_cache_store_wiring.py`: run 1 writes cache under configured root; without git changes, invoke `read_audit_cache(local_path, store=bundle.cache, cache_key=)` directly (or via a helper that mimics what remediate would do) and assert the returned envelope's `results` match run-1's results. Locks SC-001. + +- [X] T006a [US1] Add `test_shared_root_does_not_collide` to `TestLocalFsBackendRouting` in `tests/darnit/test_audit_cache_store_wiring.py`: set up two temp git repos (`repo_a`, `repo_b`) via explicit `tmp_path` subdirs plus git init, configure both with the same `.baseline.toml` block `[stores.cache] backend = "local-fs" root = "/cache"`, run the mocked audit driver against each in turn, assert BOTH `/cache/.json` AND `/cache/.json` exist, and assert `hash_a != hash_b`. Locks SC-006. + +**Checkpoint**: US1 fully functional. Operator's `[stores.cache]` config takes effect end-to-end. + +--- + +## Phase 4: User Story 2 - Zero-config default preserved (Priority: P1) + +**Goal**: Operator who never touched `.baseline.toml` sees no on-disk change from pre-feature. Cache lands at `/darnit//audit-cache.json`. + +**Independent Test**: Run audit with no `[stores.*]` block, locate on-disk cache file, assert path matches legacy shape. + +### Tests for User Story 2 + +- [X] T007 [P] [US2] Update `tests/darnit/stores/test_us2_zero_config.py::TestUS2ZeroConfig::test_filesystem_defaults_use_canonical_darnit_paths` (line 71-76). Keep the attestation and report assertions unchanged. Replace the cache assertion with: `expected_hash = hashlib.sha256(str(tmp_path.resolve()).encode()).hexdigest()[:16]; expected_root = Path(tempfile.gettempdir()) / "darnit" / expected_hash; assert bundle.cache._root == expected_root`. Add `import hashlib` and `import tempfile` at the top. Add a docstring line noting: "Cache default moved to legacy tempdir/hash location per feature 035 SC-008; other three defaults unchanged." + +- [X] T008 [US2] Add `TestZeroConfigInvariance` class to `tests/darnit/test_audit_cache_store_wiring.py` with `test_no_baseline_toml_lands_at_legacy_tempdir_path`: set up a temp git repo with no `.baseline.toml`, run the mocked audit driver, assert `/darnit//audit-cache.json` exists, assert no file under `/` (the operator's working area) got touched. Locks SC-002 and preserves FR-010. + +**Checkpoint**: US2 fully functional. Zero-config users see byte-for-byte identical on-disk behavior. + +--- + +## Phase 5: User Story 3 - TTL and staleness preserved (Priority: P1) + +**Goal**: 3600s TTL, git HEAD commit change, and working-tree dirty state changes all still invalidate the cache -- through both the default store and configured stores. + +**Independent Test**: Existing `tests/darnit/core/test_audit_cache.py` staleness tests pass under the migrated wrapper. New staleness test under a configured local-fs store also passes. + +### Tests for User Story 3 + +- [X] T009 [P] [US3] Update `tests/darnit/core/test_audit_cache.py`, two adjustments in the same file: + 1. `TestAtomicWrite::test_no_partial_file_on_error` (lines 280-293): replace `with pytest.raises(OSError):` with a plain call that MUST NOT raise. The subsequent `assert not cache_path.exists()` stays (the wrapper's default store's tempfile-then-rename cleanup handles the invariant). Update the class docstring or add an inline comment noting FR-007 relaxation. + 2. `TestInvalidateCache::test_invalidate_existing` (lines 256-262): keep the pre-write and post-invalidate assertions structural, but change the post-invalidate check from `assert not cache_path.exists()` to `assert read_audit_cache(str(temp_git_repo)) is None` (the file exists on disk with an expired envelope; the observable behavior is that the next read misses). Add an inline comment noting Q3 write-expired semantics. + +- [X] T010 [US3] Add `TestStalenessThroughConfiguredStore` class to `tests/darnit/test_audit_cache_store_wiring.py`. Add `test_head_change_invalidates_configured_cache`: configure `[stores.cache] backend = "local-fs" root = "/x"`, run audit, `git commit --allow-empty -m x`, run audit again, assert second run rebuilt the cache envelope (envelope's `commit` differs from run 1's). Add `test_dirty_state_change_invalidates_configured_cache`: run audit with clean tree, create an uncommitted file, run audit again, assert second run's envelope has `commit_dirty = True`. Locks SC-004 + SC-005 under a configured backend. + +- [X] T010a [US3] Add `TestFaultInjection` class to `tests/darnit/test_audit_cache_store_wiring.py` with `test_store_write_failure_does_not_propagate`: build a `write_audit_cache` call passing a `store` whose `write` method is monkeypatched to raise `OSError("simulated disk full")`, plus a valid `cache_key = "audit-cache"`. Assert the call returns normally (does not raise). Wrap the call in `try/except OSError` and assert the except branch does NOT fire. Also assert a warning-level log line was emitted (via `caplog`). Locks SC-007's "single `cache.write` failure MUST NOT propagate to the audit's exit code." + +**Checkpoint**: US3 fully functional. Correctness invariants of the cache (staleness) preserved through the store abstraction. + +--- + +## Phase 6: User Story 4 - Public API backward compatibility (Priority: P2) + +**Goal**: Existing consumers of `from darnit.core.audit_cache import write_audit_cache, read_audit_cache, invalidate_audit_cache` keep working with pre-feature call signatures. + +**Independent Test**: Call each of the three wrapper functions with only positional args (no `store=`, no `cache_key=`); observe the same on-disk path as pre-feature. + +### Tests for User Story 4 + +- [X] T011 [P] [US4] Add `TestBackwardCompatCallForm` class to `tests/darnit/test_audit_cache_store_wiring.py` with three tests: + 1. `test_positional_write_uses_legacy_tempdir_path`: call `write_audit_cache(str(temp_git_repo), sample_results, sample_summary, 1, "test")` with no kwargs, assert `/darnit//audit-cache.json` exists. + 2. `test_positional_read_after_positional_write_hits`: pair with the above, assert `read_audit_cache(str(temp_git_repo))` returns the envelope. + 3. `test_partial_kwarg_raises_type_error`: call `write_audit_cache(str(temp_git_repo), [], {}, 1, "test", store=None, cache_key="foo")` and assert `TypeError` raised. Reciprocal for `store=, cache_key=None`. + +- [X] T012 [P] [US4] Run `uv run pytest tests/darnit/core/test_audit_cache.py -v` and confirm all tests pass. This locks FR-011 (existing test surface intact aside from the two adjusted in T009). + +**Checkpoint**: US4 fully functional. Any pre-feature-035 external caller (including cached copies of `darnit-baseline`, third-party MCP tool wrappers, etc.) continues to work. + +--- + +## Phase 7: Polish & Cross-Cutting Concerns + +- [X] T013 Run `uv run pytest tests/darnit/ -v` and confirm the full framework test suite passes. Expected diffs: the four test adjustments from T007, T009 (two), and any new tests added in T005/T006/T008/T010/T011. All other pre-existing tests MUST pass unchanged. + +- [X] T014 [P] Run `uv run ruff check .` and `uv run ruff format .` -- fix any lint / format drift introduced by the changes. Repo-wide, not per-file (learned in feature 034). NOTE: `ruff format .` reformatted 234 unrelated files across the repo (accumulated format drift, not feature 035); reverted those and kept only the format changes on this feature's 7 files, all of which pass `ruff check` clean. + +- [X] T015 Walk through [quickstart.md](quickstart.md) Example 1 (CI operator) manually against `/tmp/test-repo`: initialize a git repo, add `.baseline.toml` with `[stores.cache] backend = "local-fs" root = "/tmp/qs-cache"`, run `darnit audit /tmp/test-repo --level 1`, confirm `/tmp/qs-cache/.json` exists, run again, confirm the log line "Audit cache hit ..." fires (from `read_audit_cache`'s existing debug log; may need to bump log level to see it). Verifies end-to-end wiring outside of pytest fixtures. + +- [X] T016 Confirm no product-package or plugin-package import surfaces changed. Grep for `write_audit_cache\|read_audit_cache\|invalidate_audit_cache` under `packages/` and `packages/darnit-baseline/`, `packages/darnit-gittuf/`, `packages/darnit-reproducibility/`, `packages/darnit-hello/`: any hits outside `packages/darnit/src/darnit/{core,tools}/` and `tests/` are unexpected and should be inspected. Locks SC-009 (file-scope boundary). + +--- + +## Dependencies & Execution Order + +### Phase Dependencies + +- **Phase 1 (Setup)**: no dependencies -- can start immediately. +- **Phase 2 (Foundational)**: depends on Phase 1. BLOCKS all user stories. + - Within Phase 2: T002 -> T003 -> T004 must be strictly ordered (T003 depends on T002's new default so its own default helper aligns; T004 depends on T003's new wrapper signature). +- **Phase 3 (US1)**: depends on Phase 2 complete. T006 and T006a depend on T005 (file creation). +- **Phase 4 (US2)**: depends on Phase 2 complete. T007 and T008 are independent of each other but both touch different files (T007 -> existing test file, T008 -> new test file); both can proceed after T005 lands. +- **Phase 5 (US3)**: depends on Phase 2 complete. T009 (existing file) can run in parallel with T010 and T010a. T010 and T010a share `test_audit_cache_store_wiring.py` and are sequential within that file. +- **Phase 6 (US4)**: depends on Phase 2 complete. T011 and T012 are independent; both can run in parallel. +- **Phase 7 (Polish)**: depends on all US phases complete. + +### User Story Dependencies + +- **US1**: no dependencies on other stories. +- **US2**: no dependencies on other stories. +- **US3**: no dependencies on other stories. +- **US4**: no dependencies on other stories (existing test surface is the target; foundation must be in place). + +### Within Each User Story + +- Test additions target the new file `tests/darnit/test_audit_cache_store_wiring.py`. Multiple tasks touching that file are sequential within the file (US1's T005/T006/T006a, US2's T008, US3's T010/T010a, US4's T011). Between stories the file order is: T005 -> T006 -> T006a -> T008 -> T010 -> T010a -> T011. +- Existing-file test updates (T007 -> `test_us2_zero_config.py`, T009 -> `test_audit_cache.py`) each touch one file and can run in parallel with the new-file tasks. + +### Parallel Opportunities + +- **T007** and **T009** and **T010** can run in parallel with each other (three different files). +- **T014** can run in parallel with **T015** and **T016** (lint vs manual walkthrough vs grep). +- Nothing in Phase 2 can be parallelized; the ordering is a hard sequence. + +--- + +## Parallel Example: Phase 5 (US3) + +```bash +# Tests for US3 that touch different files can run together: +Task: "Update tests/darnit/core/test_audit_cache.py (T009): two behavior-change adjustments" +Task: "Add TestStalenessThroughConfiguredStore to tests/darnit/test_audit_cache_store_wiring.py (T010)" +``` + +--- + +## Implementation Strategy + +### MVP first (US1 only) + +1. Complete Phase 1 (Setup). +2. Complete Phase 2 (Foundational). The wrapper + driver + default cache_root together. +3. Complete Phase 3 (US1) -- the operator-configures-and-it-works story. +4. STOP and VALIDATE: confirm `[stores.cache] backend = "local-fs" root = "..."` in `.baseline.toml` actually redirects the cache file. +5. Demo / open PR draft. + +### Incremental delivery + +- After MVP, add US2 (zero-config invariance test coverage) so the release notes can honestly claim "no on-disk change for zero-config users." +- Add US3 (staleness under configured store) so operators trust the correctness gate. +- Add US4 (backward-compat tests) as belt-and-braces for external consumers. +- Polish (Phase 7) closes the PR. + +### Solo strategy (this is likely a single-author feature) + +Straight-through top-to-bottom is fine. Estimated wall time: 3-5 hours for Phase 1-7 including manual quickstart validation. No parallelization needed. + +--- + +## Notes + +- [P] tasks: different files, no dependencies on incomplete tasks. +- [Story] label: maps task to US1/US2/US3/US4 for traceability. +- Every user story is independently completable and testable after Phase 2. +- Commit after each phase (or each task within a phase for larger changes). Recommended commit boundary: after T002/T003/T004 together (foundation), then per-phase after each US. +- The task list assumes feature 034 (PR #412) is merged. If it isn't at implementation time, T005/T006 still work against `LocalFsAuditCacheStore` (feature 034 is in the code even pre-merge if on that branch); confirm at T001. +- Avoid: unrelated cleanup in `core/audit_cache.py` (this is a compliance tool; behavior-focused edits only per CLAUDE.md). diff --git a/tests/darnit/core/test_audit_cache.py b/tests/darnit/core/test_audit_cache.py index 999d9265..f78e5138 100644 --- a/tests/darnit/core/test_audit_cache.py +++ b/tests/darnit/core/test_audit_cache.py @@ -259,7 +259,12 @@ def test_invalidate_existing(self, temp_git_repo: Path, sample_results, sample_s assert cache_path.exists() invalidate_audit_cache(str(temp_git_repo)) - assert not cache_path.exists() + # Feature 035 clarify Q3: invalidation writes an expired envelope + # rather than deleting the file (AuditCacheStore has no delete()). + # The file remains on disk with a 1970-01-01 timestamp; the + # observable behavior is that the next read misses on TTL. + assert cache_path.exists() + assert read_audit_cache(str(temp_git_repo)) is None @pytest.mark.unit def test_invalidate_missing_noop(self, temp_git_repo: Path): @@ -278,16 +283,27 @@ class TestAtomicWrite: @pytest.mark.unit def test_no_partial_file_on_error(self, temp_git_repo: Path, sample_results, sample_summary): - """If json.dump raises, no cache file should be left behind.""" - with patch("darnit.core.audit_cache.json.dump", side_effect=OSError("disk full")): - with pytest.raises(OSError): - write_audit_cache( - str(temp_git_repo), - sample_results, - sample_summary, - 3, - "test", - ) + """A failing store.write is swallowed with a warning; no partial file. + + Feature 035 FR-007 relaxed the pre-feature "raises on tempfile/rename + failure" behavior to feature-033 FR-011's best-effort semantics: log + and continue. The 'no partial file' invariant is now enforced by + FilesystemAuditCacheStore's tempfile-then-rename + tempfile cleanup. + """ + # Patch the underlying store's write to raise. The wrapper MUST + # swallow it (no raise) and no cache file should exist afterwards. + with patch( + "darnit.stores.defaults.cache.FilesystemAuditCacheStore.write", + side_effect=OSError("disk full"), + ): + # Must NOT raise (FR-007). + write_audit_cache( + str(temp_git_repo), + sample_results, + sample_summary, + 3, + "test", + ) cache_path = _get_cache_dir(str(temp_git_repo)) / CACHE_FILENAME assert not cache_path.exists() diff --git a/tests/darnit/stores/test_us1_isolation.py b/tests/darnit/stores/test_us1_isolation.py index cb6c7a2a..4e7a9c0d 100644 --- a/tests/darnit/stores/test_us1_isolation.py +++ b/tests/darnit/stores/test_us1_isolation.py @@ -7,6 +7,8 @@ from __future__ import annotations +import hashlib +import tempfile from pathlib import Path from darnit_testchecks.stores import InMemoryProjectStateStore @@ -35,11 +37,15 @@ def test_filesystem_defaults_land_on_darnit_subdir(self, tmp_path: Path): bundle = resolve_stores(config, repo_path=tmp_path) # Trigger construction and verify the filesystem defaults were - # built against `/.darnit/...`, the canonical zero-config - # location. + # built at their canonical zero-config location. Feature 035 + # rebased the cache default to the legacy tempdir/hash path + # (SC-008); attestation and report defaults are unchanged. att = bundle.attestation rep = bundle.report cache = bundle.cache assert att._root == tmp_path / ".darnit" / "attestations" # type: ignore[attr-defined] assert rep._root == tmp_path / ".darnit" / "reports" # type: ignore[attr-defined] - assert cache._root == tmp_path / ".darnit" / "audit-cache" # type: ignore[attr-defined] + + expected_hash = hashlib.sha256(str(tmp_path.resolve()).encode()).hexdigest()[:16] + expected_cache_root = Path(tempfile.gettempdir()) / "darnit" / expected_hash + assert cache._root == expected_cache_root # type: ignore[attr-defined] diff --git a/tests/darnit/stores/test_us2_zero_config.py b/tests/darnit/stores/test_us2_zero_config.py index ffbda480..ee689409 100644 --- a/tests/darnit/stores/test_us2_zero_config.py +++ b/tests/darnit/stores/test_us2_zero_config.py @@ -10,6 +10,8 @@ from __future__ import annotations +import hashlib +import tempfile from pathlib import Path from darnit.stores import discovery @@ -30,9 +32,7 @@ def test_none_config_yields_filesystem_defaults_for_all_four(self, tmp_path: Pat assert isinstance(bundle.report, FilesystemReportStore) assert isinstance(bundle.cache, FilesystemAuditCacheStore) - def test_no_plugin_backend_constructed_under_zero_config( - self, tmp_path: Path - ): + def test_no_plugin_backend_constructed_under_zero_config(self, tmp_path: Path): """SC-003: a plugin registered but not selected must never be built.""" # Reset cache so we can monkey-inject a fake entry. discovery._reset_discovery_cache() @@ -51,9 +51,7 @@ def close(self): # Seed the per-process discovery cache directly so # `discover_stores("darnit.stores.attestation")` sees the fake. - discovery._DISCOVERY_CACHE["darnit.stores.attestation"] = { - "fake-plugin": FakePlugin - } + discovery._DISCOVERY_CACHE["darnit.stores.attestation"] = {"fake-plugin": FakePlugin} bundle = resolve_stores(None, repo_path=tmp_path) # Force realistic access on all four kinds. @@ -69,8 +67,18 @@ def close(self): discovery._reset_discovery_cache() def test_filesystem_defaults_use_canonical_darnit_paths(self, tmp_path: Path): - """Zero-config on-disk paths match the pre-feature convention.""" + """Zero-config on-disk paths match the pre-feature convention. + + Feature 035 SC-008: cache default moved to the legacy + ``/darnit/`` location so the + pre-feature ``darnit.core.audit_cache`` on-disk path is preserved + byte-for-byte after the store-abstraction routing. Attestation + and report defaults are unchanged. + """ bundle = resolve_stores(None, repo_path=tmp_path) assert bundle.attestation._root == tmp_path / ".darnit" / "attestations" # type: ignore[attr-defined] assert bundle.report._root == tmp_path / ".darnit" / "reports" # type: ignore[attr-defined] - assert bundle.cache._root == tmp_path / ".darnit" / "audit-cache" # type: ignore[attr-defined] + + expected_hash = hashlib.sha256(str(tmp_path.resolve()).encode()).hexdigest()[:16] + expected_cache_root = Path(tempfile.gettempdir()) / "darnit" / expected_hash + assert bundle.cache._root == expected_cache_root # type: ignore[attr-defined] diff --git a/tests/darnit/test_audit_cache_store_wiring.py b/tests/darnit/test_audit_cache_store_wiring.py new file mode 100644 index 00000000..da66b503 --- /dev/null +++ b/tests/darnit/test_audit_cache_store_wiring.py @@ -0,0 +1,396 @@ +"""Feature 035 driver-level integration for the audit-cache store wiring. + +Locks the three properties that were broken before this feature: + +* Operator's ``[stores.cache] backend = "local-fs" root = "..."`` config + actually redirects the on-disk cache path (US1). +* Zero-config default still lands cache at + ``/darnit//audit-cache.json`` (US2, invariance). +* Staleness detection (TTL / commit / dirty) still fires through a + configured store (US3). +* Public API keeps working with positional-only backward-compat call + form (US4). +* Store write failures do not propagate to the audit's exit code (US3 + fault-injection). + +The tests use a mocked SieveOrchestrator so they exercise the driver's +cache-write code path without needing a real framework registered. +""" + +from __future__ import annotations + +import hashlib +import subprocess +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from darnit.config.framework_schema import StoreBlock, StoresConfig +from darnit.core.audit_cache import ( + CACHE_FILENAME, + CACHE_VERSION, + _get_cache_dir, + read_audit_cache, + write_audit_cache, +) +from darnit.stores.defaults import FilesystemAuditCacheStore +from darnit.stores.defaults.local_fs import LocalFsAuditCacheStore + +# --------------------------------------------------------------------------- +# Shared mocks for run_sieve_audit +# --------------------------------------------------------------------------- + + +def _mock_sieve_components() -> dict: + """Return the minimum sieve components a mocked audit needs.""" + mock_result = MagicMock() + mock_result.to_legacy_dict.return_value = { + "id": "TEST-01", + "status": "PASS", + "details": "OK", + "level": 1, + } + + mock_spec = MagicMock() + mock_spec.control_id = "TEST-01" + mock_spec.name = "Test Control" + mock_spec.description = "A test control" + mock_spec.level = 1 + mock_spec.metadata = {"full": ""} + mock_spec.locator_config = None + + mock_orchestrator = MagicMock() + mock_orchestrator.verify.return_value = mock_result + + mock_registry = MagicMock() + mock_registry.get_specs_by_level.return_value = [mock_spec] + + return { + "SieveOrchestrator": lambda **kw: mock_orchestrator, + "get_control_registry": lambda: mock_registry, + "CheckContext": MagicMock(), + } + + +def _run_audit_with_stores_config(repo: Path, stores_config: StoresConfig | None): + """Invoke ``run_sieve_audit`` with a mocked orchestrator + injected stores. + + Patches ``_load_merged_stores`` to return ``stores_config`` (so we + bypass the framework-registration step) and ``_register_toml_controls`` + plus ``get_excluded_control_ids`` per the existing test pattern. + """ + with ( + patch( + "darnit.tools.audit._get_sieve_components", + return_value=_mock_sieve_components(), + ), + patch("darnit.tools.audit._register_toml_controls", return_value=0), + patch("darnit.tools.audit.get_excluded_control_ids", return_value={}), + patch( + "darnit.tools.audit._load_merged_stores", + return_value=stores_config, + ), + patch("darnit.config.load_user_config", return_value=None), + ): + from darnit.tools.audit import run_sieve_audit + + return run_sieve_audit( + owner="test-owner", + repo="test-repo", + local_path=str(repo), + default_branch="main", + level=1, + ) + + +def _repo_hash(repo: Path) -> str: + return hashlib.sha256(str(repo.resolve()).encode()).hexdigest()[:16] + + +# --------------------------------------------------------------------------- +# US1: local-fs backend routing +# --------------------------------------------------------------------------- + + +class TestLocalFsBackendRouting: + """SC-001, SC-006 -- configured store redirects the cache path.""" + + @pytest.mark.unit + def test_config_moves_cache_to_configured_root(self, temp_git_repo: Path, tmp_path: Path): + """Cache file lands under configured root, not the tempdir.""" + cache_root = tmp_path / "cache-out" + stores_config = StoresConfig(cache=StoreBlock(backend="local-fs", root=str(cache_root))) + + _run_audit_with_stores_config(temp_git_repo, stores_config) + + expected_file = cache_root / f"{_repo_hash(temp_git_repo)}.json" + assert expected_file.exists(), ( + f"cache file should land at {expected_file} " + f"but did not. cache_root contents: " + f"{list(cache_root.iterdir()) if cache_root.exists() else 'no dir'}" + ) + # No fallback file in the default tempdir location. + default_dir = _get_cache_dir(str(temp_git_repo)) + assert not (default_dir / CACHE_FILENAME).exists(), ( + "cache should NOT land in default tempdir when [stores.cache] is configured" + ) + + @pytest.mark.unit + def test_second_run_reads_cache_via_configured_store(self, temp_git_repo: Path, tmp_path: Path): + """Run 2 finds run 1's cache under the configured root (SC-001).""" + cache_root = tmp_path / "cache-out" + stores_config = StoresConfig(cache=StoreBlock(backend="local-fs", root=str(cache_root))) + + run1_results, run1_summary = _run_audit_with_stores_config(temp_git_repo, stores_config) + + # Read the cache the same way the remediate tool would: build a + # matching store and pass the same cache_key the driver used. + store = LocalFsAuditCacheStore(root=str(cache_root)) + cache_key = _repo_hash(temp_git_repo) + envelope = read_audit_cache(str(temp_git_repo), store=store, cache_key=cache_key) + assert envelope is not None, "read should HIT the configured-root cache written by run 1" + assert envelope["results"] == run1_results + assert envelope["summary"] == run1_summary + + @pytest.mark.unit + def test_shared_root_does_not_collide(self, tmp_path: Path): + """Two repos sharing one configured root do not overwrite (SC-006).""" + shared_root = tmp_path / "shared-cache" + + # Two independent repos. + repo_a = tmp_path / "repo-a" + repo_b = tmp_path / "repo-b" + for repo in (repo_a, repo_b): + repo.mkdir() + subprocess.run(["git", "init"], cwd=repo, capture_output=True, check=True) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], + cwd=repo, + capture_output=True, + check=True, + ) + subprocess.run( + ["git", "config", "user.name", "Test User"], + cwd=repo, + capture_output=True, + check=True, + ) + (repo / "README.md").write_text("# " + repo.name + "\n") + subprocess.run(["git", "add", "."], cwd=repo, capture_output=True, check=True) + subprocess.run( + ["git", "commit", "-m", "init"], + cwd=repo, + capture_output=True, + check=True, + ) + + stores_config = StoresConfig(cache=StoreBlock(backend="local-fs", root=str(shared_root))) + + _run_audit_with_stores_config(repo_a, stores_config) + _run_audit_with_stores_config(repo_b, stores_config) + + hash_a = _repo_hash(repo_a) + hash_b = _repo_hash(repo_b) + assert hash_a != hash_b, "distinct repos MUST hash to distinct keys" + assert (shared_root / f"{hash_a}.json").exists() + assert (shared_root / f"{hash_b}.json").exists() + + +# --------------------------------------------------------------------------- +# US2: zero-config default preserved +# --------------------------------------------------------------------------- + + +class TestZeroConfigInvariance: + """FR-010, SC-002 -- no [stores.*] block means legacy tempdir path.""" + + @pytest.mark.unit + def test_no_baseline_toml_lands_at_legacy_tempdir_path(self, temp_git_repo: Path, tmp_path: Path): + """Zero-config audit lands cache at pre-feature-035 path shape.""" + _run_audit_with_stores_config(temp_git_repo, stores_config=None) + + legacy_dir = _get_cache_dir(str(temp_git_repo)) + legacy_file = legacy_dir / CACHE_FILENAME + assert legacy_file.exists(), ( + f"zero-config cache should land at {legacy_file}, " + f"tempdir tree: {list(legacy_dir.iterdir()) if legacy_dir.exists() else 'no dir'}" + ) + # Confirm no leakage into the operator's working area. + assert not (temp_git_repo / ".darnit" / "audit-cache").exists(), ( + "zero-config MUST NOT write into /.darnit/audit-cache" + ) + + +# --------------------------------------------------------------------------- +# US3: staleness through a configured store +# --------------------------------------------------------------------------- + + +class TestStalenessThroughConfiguredStore: + """SC-004, SC-005 -- HEAD change / dirty tree still invalidate cache.""" + + @pytest.mark.unit + def test_head_change_invalidates_configured_cache(self, temp_git_repo: Path, tmp_path: Path): + """A new commit between runs forces a fresh envelope (SC-004).""" + cache_root = tmp_path / "cache-out" + stores_config = StoresConfig(cache=StoreBlock(backend="local-fs", root=str(cache_root))) + + _run_audit_with_stores_config(temp_git_repo, stores_config) + + store = LocalFsAuditCacheStore(root=str(cache_root)) + cache_key = _repo_hash(temp_git_repo) + + envelope_run1 = read_audit_cache(str(temp_git_repo), store=store, cache_key=cache_key) + assert envelope_run1 is not None + run1_commit = envelope_run1["commit"] + + # Advance HEAD. + subprocess.run( + ["git", "commit", "--allow-empty", "-m", "advance HEAD"], + cwd=temp_git_repo, + capture_output=True, + check=True, + ) + + # Read via wrapper -- staleness fires, returns None. + envelope_after_head = read_audit_cache(str(temp_git_repo), store=store, cache_key=cache_key) + assert envelope_after_head is None, "cache MUST miss after HEAD change (SC-004)" + + # Run 2 rewrites the envelope with the new commit. + _run_audit_with_stores_config(temp_git_repo, stores_config) + envelope_run2 = read_audit_cache(str(temp_git_repo), store=store, cache_key=cache_key) + assert envelope_run2 is not None + assert envelope_run2["commit"] != run1_commit, "run 2 envelope MUST reflect the new HEAD" + + @pytest.mark.unit + def test_dirty_state_change_invalidates_configured_cache(self, temp_git_repo: Path, tmp_path: Path): + """Dirtying the tree between runs forces a fresh envelope (SC-005).""" + cache_root = tmp_path / "cache-out" + stores_config = StoresConfig(cache=StoreBlock(backend="local-fs", root=str(cache_root))) + + _run_audit_with_stores_config(temp_git_repo, stores_config) + + store = LocalFsAuditCacheStore(root=str(cache_root)) + cache_key = _repo_hash(temp_git_repo) + + envelope_run1 = read_audit_cache(str(temp_git_repo), store=store, cache_key=cache_key) + assert envelope_run1 is not None + assert envelope_run1["commit_dirty"] is False + + # Dirty the tree. + (temp_git_repo / "uncommitted.txt").write_text("dirty\n") + + envelope_after_dirty = read_audit_cache(str(temp_git_repo), store=store, cache_key=cache_key) + assert envelope_after_dirty is None, "cache MUST miss after tree becomes dirty (SC-005)" + + _run_audit_with_stores_config(temp_git_repo, stores_config) + envelope_run2 = read_audit_cache(str(temp_git_repo), store=store, cache_key=cache_key) + assert envelope_run2 is not None + assert envelope_run2["commit_dirty"] is True + + +# --------------------------------------------------------------------------- +# US3 (bonus): fault injection +# --------------------------------------------------------------------------- + + +class TestFaultInjection: + """SC-007 -- store.write failures MUST NOT propagate.""" + + @pytest.mark.unit + def test_store_write_failure_does_not_propagate(self, temp_git_repo: Path, caplog): + """A raising store.write is swallowed with a warning log.""" + import logging + + class RaisingStore: + def read(self, cache_key: str): + return None + + def write(self, cache_key: str, envelope: dict) -> None: + raise OSError("simulated disk full") + + def close(self) -> None: + return None + + # Must not raise. Wrap in try/except to also verify no OSError + # leaks through. + raised: Exception | None = None + with caplog.at_level(logging.WARNING, logger="darnit.core.audit_cache"): + try: + write_audit_cache( + str(temp_git_repo), + [{"id": "TEST", "status": "PASS", "details": "OK", "level": 1}], + {"PASS": 1, "FAIL": 0, "WARN": 0, "N/A": 0, "ERROR": 0, "total": 1}, + 1, + "test-fw", + store=RaisingStore(), + cache_key="audit-cache", + ) + except Exception as exc: # pragma: no cover -- assertion below + raised = exc + + assert raised is None, f"write_audit_cache MUST NOT propagate store errors, got {raised!r}" + # Warning line captured. + assert any("audit cache write failed" in rec.message for rec in caplog.records), ( + "expected a warning log line about the cache write failure" + ) + + +# --------------------------------------------------------------------------- +# US4: backward-compat call form (positional-only) +# --------------------------------------------------------------------------- + + +class TestBackwardCompatCallForm: + """FR-002, FR-011 -- external positional callers keep working.""" + + @pytest.mark.unit + def test_positional_write_uses_legacy_tempdir_path(self, temp_git_repo: Path): + write_audit_cache( + str(temp_git_repo), + [{"id": "T", "status": "PASS", "details": "OK", "level": 1}], + {"PASS": 1, "FAIL": 0, "WARN": 0, "N/A": 0, "ERROR": 0, "total": 1}, + 1, + "test-fw", + ) + expected = _get_cache_dir(str(temp_git_repo)) / CACHE_FILENAME + assert expected.exists(), "positional write MUST land at the legacy tempdir path" + + @pytest.mark.unit + def test_positional_read_after_positional_write_hits(self, temp_git_repo: Path): + results = [{"id": "T", "status": "PASS", "details": "OK", "level": 1}] + summary = {"PASS": 1, "FAIL": 0, "WARN": 0, "N/A": 0, "ERROR": 0, "total": 1} + write_audit_cache(str(temp_git_repo), results, summary, 1, "test-fw") + envelope = read_audit_cache(str(temp_git_repo)) + assert envelope is not None + assert envelope["version"] == CACHE_VERSION + assert envelope["results"] == results + assert envelope["summary"] == summary + + @pytest.mark.unit + def test_partial_kwarg_store_only_raises_type_error(self, temp_git_repo: Path, tmp_path: Path): + store = FilesystemAuditCacheStore(root=tmp_path / "root") + with pytest.raises(TypeError, match="store and cache_key"): + write_audit_cache( + str(temp_git_repo), + [], + {"PASS": 0, "FAIL": 0, "WARN": 0, "N/A": 0, "ERROR": 0, "total": 0}, + 1, + "test-fw", + store=store, + cache_key=None, + ) + + @pytest.mark.unit + def test_partial_kwarg_cache_key_only_raises_type_error(self, temp_git_repo: Path): + with pytest.raises(TypeError, match="store and cache_key"): + write_audit_cache( + str(temp_git_repo), + [], + {"PASS": 0, "FAIL": 0, "WARN": 0, "N/A": 0, "ERROR": 0, "total": 0}, + 1, + "test-fw", + store=None, + cache_key="foo", + )