From cf4d41d68112c3228d5b9fe05e1946e20194118e Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Sun, 6 Sep 2026 11:56:56 -0400 Subject: [PATCH] fix(determinism): Tier 1 -- deterministic ops must be deterministic (#418) Same repo + same command should produce the same audit / remediation output every run. Fixes the seven concrete Tier 1 gaps surfaced in the determinism survey. Filesystem iteration (audit + context): - context/auto_detect.py:78 -- detect_ci_provider() now sorts os.listdir results before scanning for .yml / .yaml siblings. - context/auto_detect.py:309 -- detect_has_subprojects() sorts iterdir children even though len() is order-independent today; future first-match/slicing changes stay safe by construction. - sieve/builtin_handlers.py:96 -- _walk_depth_limited sorts dirnames in-place before yielding so "first match wins" downstream is stable. - sieve/builtin_handlers.py:135, 449 -- glob.glob results sorted at both call sites; taking matches[0] is now deterministic across filesystems that don't sort glob output by default. Wall-clock injection into remediation templates: - remediation/executor.py:_get_template_context -- DATE dropped (no template referenced it, day-per-day drift cluttered PR diffs for identical inputs). YEAR kept (LICENSE templates use it; year-per-year cadence is slow enough to stay stable within a calendar year). New now_provider kwarg on RemediationExecutor lets tests inject a fixed clock so YEAR's derivation is deterministic under test. List ordering in remediation templates: - remediation/executor.py -- list-valued context (e.g., maintainers) is sorted before " ".join, so upstream ordering drift (dict iteration, API-response order) doesn't drift rendered output. Non-atomic writes: - sieve/builtin_handlers.py -- new _atomic_write_text helper (tempfile-then-rename in same directory, cleans tempfile on failure). file_create_handler:837 and yaml_inject_handler:983 now route through it; a crash mid-write can no longer leave a partial file. Same invariant FilesystemAuditCacheStore uses (feature 033). Tests: - tests/darnit/test_determinism_tier1.py -- 10 new tests covering each guarantee: atomic write with no partial file on os.replace failure, file_create no-partial-on-failure, _walk_depth_limited sorted visitation, file_exists glob first-match stability, now_provider injection, DATE-field absence, list-value sort determinism, and detect_ci_provider sorted-listdir consumption. Full framework + baseline test suite: 2808 passed, 13 skipped, 0 failed. --- .../darnit/src/darnit/context/auto_detect.py | 16 +- .../darnit/src/darnit/remediation/executor.py | 27 ++- .../src/darnit/sieve/builtin_handlers.py | 50 ++++- tests/darnit/test_determinism_tier1.py | 209 ++++++++++++++++++ 4 files changed, 281 insertions(+), 21 deletions(-) create mode 100644 tests/darnit/test_determinism_tier1.py diff --git a/packages/darnit/src/darnit/context/auto_detect.py b/packages/darnit/src/darnit/context/auto_detect.py index b96bc7c9..a6cc4bf0 100644 --- a/packages/darnit/src/darnit/context/auto_detect.py +++ b/packages/darnit/src/darnit/context/auto_detect.py @@ -75,7 +75,10 @@ def detect_ci_provider(local_path: str) -> str | None: # For directories (e.g. .github/workflows), check it has files if os.path.isdir(full_path): try: - entries = os.listdir(full_path) + # sorted() so a repo with multiple workflow files always + # scans them in the same order across runs. Determinism + # Tier 1 (#418). + entries = sorted(os.listdir(full_path)) if any( e.endswith((".yml", ".yaml")) for e in entries ): @@ -305,10 +308,13 @@ def detect_has_subprojects(local_path: str) -> bool | None: d = p / dirname if d.is_dir(): try: - children = [ - c for c in d.iterdir() - if c.is_dir() and not c.name.startswith(".") - ] + # sorted() so iteration order is stable across runs even if + # a future change replaces the len() check with slice / + # first-match semantics. Determinism Tier 1 (#418). + children = sorted( + (c for c in d.iterdir() if c.is_dir() and not c.name.startswith(".")), + key=lambda c: c.name, + ) if len(children) >= 2: return True except OSError: diff --git a/packages/darnit/src/darnit/remediation/executor.py b/packages/darnit/src/darnit/remediation/executor.py index c2024cc8..2becf3af 100644 --- a/packages/darnit/src/darnit/remediation/executor.py +++ b/packages/darnit/src/darnit/remediation/executor.py @@ -30,6 +30,8 @@ from typing import TYPE_CHECKING, Any if TYPE_CHECKING: + from collections.abc import Callable + from jinja2 import Environment from darnit.config.framework_schema import ( @@ -107,8 +109,9 @@ class RemediationExecutor: - << REPO >> - Repository name - << BRANCH >> - Default branch - << PATH >> - Local repository path - - << YEAR >> - Current year - - << DATE >> - Current date (ISO format) + - << YEAR >> - Current year (used by LICENSE templates; year-per-year + cadence is slow enough that PR diffs stay stable within a calendar + year -- see Determinism Tier 1 note in _get_template_context) - << CONTROL >> - Control ID being remediated - << context.KEY >> - Confirmed project context values - << project.KEY >> - Values from .project/project.yaml @@ -127,6 +130,7 @@ def __init__( project_values: dict[str, Any] | None = None, scan_values: dict[str, Any] | None = None, framework_path: str | None = None, + now_provider: Callable[[], datetime] | None = None, ): """Initialize the executor. @@ -144,6 +148,10 @@ def __init__( framework_path: Absolute path to the framework TOML file. Template ``file`` references are resolved relative to this file's directory. Falls back to ``local_path`` when None. + now_provider: Optional callable returning the "current" datetime, + used to derive ``<< YEAR >>`` in template output. Defaults + to :func:`datetime.now`. Parameterized so tests can inject + a fixed clock (Determinism Tier 1, #418). """ self.local_path = os.path.abspath(local_path) self.templates = templates or {} @@ -152,6 +160,7 @@ def __init__( self._context_values = context_values or {} self._project_values = project_values or {} self._scan_values = scan_values or {} + self._now_provider = now_provider or datetime.now # Auto-detect owner/repo if not provided if not owner or not repo: @@ -175,25 +184,31 @@ def _get_template_context(self, control_id: str) -> dict[str, Any]: Jinja2 templates access these as e.g. ``<< REPO >>`` or ``<< context.maintainers >>``. """ - now = datetime.now() + # YEAR is derived from now_provider (test-injectable). DATE was + # dropped: no template referenced it, and its day-per-day drift + # was cluttering PR diffs for identical inputs run on different + # days. Determinism Tier 1 (#418). + now = self._now_provider() ctx: dict[str, Any] = { "OWNER": self.owner or "", "REPO": self.repo or "", "BRANCH": self.default_branch, "PATH": self.local_path, "YEAR": str(now.year), - "DATE": now.strftime("%Y-%m-%d"), "CONTROL": control_id, } - # Build nested context/project/scan namespaces + # Build nested context/project/scan namespaces. List values are + # sorted before joining so upstream ordering (dict iteration, + # API-response order) does not drift the rendered output across + # runs. Determinism Tier 1 (#418). context_ns: dict[str, str] = {} if self._context_values: for key, value in self._context_values.items(): if isinstance(value, str): context_ns[key] = value elif isinstance(value, list): - context_ns[key] = " ".join(str(v) for v in value) + context_ns[key] = " ".join(sorted(str(v) for v in value)) elif value is not None: context_ns[key] = str(value) ctx["context"] = context_ns diff --git a/packages/darnit/src/darnit/sieve/builtin_handlers.py b/packages/darnit/src/darnit/sieve/builtin_handlers.py index ba6cadd8..8641982a 100644 --- a/packages/darnit/src/darnit/sieve/builtin_handlers.py +++ b/packages/darnit/src/darnit/sieve/builtin_handlers.py @@ -22,6 +22,7 @@ import os import re import subprocess +import tempfile from typing import Any from .handler_registry import ( @@ -38,6 +39,29 @@ # ============================================================================= MCP_DEFAULT_TIMEOUT_SECONDS: float = 60.0 + + +def _atomic_write_text(path: str, content: str) -> None: + """Write ``content`` to ``path`` atomically via tempfile-then-rename. + + Determinism Tier 1 (#418): direct ``open(path, "w").write(content)`` + leaves a partial file behind if the process crashes or the disk fills + mid-write. Tempfile in the same directory + ``os.replace`` gives us + the same "either fully written or absent" invariant that + :class:`FilesystemAuditCacheStore` uses (feature 033). + """ + directory = os.path.dirname(path) or "." + fd, tmp = tempfile.mkstemp(dir=directory, prefix=".darnit-write-", suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(content) + os.replace(tmp, path) + except Exception: + try: + os.unlink(tmp) + except OSError: + pass + raise """Per-call timeout for `handler = "mcp"` passes when the pass omits `timeout`. Spec FR-002 (clarified 2026-08-16). Individual passes MAY override via @@ -95,8 +119,10 @@ def _walk_depth_limited(root: str, max_depth: int): return for dirpath, dirnames, _files in os.walk(root_abs): depth = dirpath[len(root_abs) :].count(os.sep) - # Prune in-place so os.walk skips them (matches os.walk's contract) - dirnames[:] = [d for d in dirnames if d not in _FILE_DISCOVERY_PRUNE_DIRS] + # Prune in-place so os.walk skips them (matches os.walk's contract). + # Sort so os.walk visits subdirs deterministically -- "first match + # wins" semantics downstream depend on this. Determinism Tier 1 (#418). + dirnames[:] = sorted(d for d in dirnames if d not in _FILE_DISCOVERY_PRUNE_DIRS) if depth >= max_depth: # Don't descend further; stop yielding deeper dirs dirnames.clear() @@ -132,7 +158,9 @@ def file_exists_handler(config: dict[str, Any], context: HandlerContext) -> Hand if "*" in pattern: import glob - matches = glob.glob(os.path.join(context.local_path, pattern)) + # sorted() so "first match wins" is stable across filesystems. + # Determinism Tier 1 (#418). + matches = sorted(glob.glob(os.path.join(context.local_path, pattern))) if matches: found = matches[0] rel_path = os.path.relpath(found, context.local_path) @@ -449,9 +477,13 @@ def _resolve_regex_files( for file_pattern in files_list: if "*" in file_pattern or "?" in file_pattern: # Glob patterns: always use glob.glob; max_depth does not apply. - matches = globmod.glob( - os.path.join(context.local_path, file_pattern), - recursive=True, + # sorted() so downstream ordering is stable across filesystems. + # Determinism Tier 1 (#418). + matches = sorted( + globmod.glob( + os.path.join(context.local_path, file_pattern), + recursive=True, + ) ) resolved.extend(m for m in matches if os.path.isfile(m)) elif max_depth > 0: @@ -834,8 +866,7 @@ def file_create_handler(config: dict[str, Any], context: HandlerContext) -> Hand try: os.makedirs(os.path.dirname(full_path), exist_ok=True) - with open(full_path, "w", encoding="utf-8") as f: - f.write(content) + _atomic_write_text(full_path, content) except OSError as e: return HandlerResult( status=HandlerResultStatus.ERROR, @@ -980,8 +1011,7 @@ def yaml_inject_handler(config: dict[str, Any], context: HandlerContext) -> Hand lines.insert(insert_idx, injection.rstrip()) try: - with open(filepath, "w", encoding="utf-8") as f: - f.write("\n".join(lines)) + _atomic_write_text(filepath, "\n".join(lines)) modified.append(os.path.relpath(filepath, context.local_path)) except OSError: continue diff --git a/tests/darnit/test_determinism_tier1.py b/tests/darnit/test_determinism_tier1.py new file mode 100644 index 00000000..c1bb9689 --- /dev/null +++ b/tests/darnit/test_determinism_tier1.py @@ -0,0 +1,209 @@ +"""Regression tests for Determinism Tier 1 (#418). + +Locks the concrete guarantees the Tier 1 fix cluster promises: + +* Filesystem iteration is sorted at every first-match / count site + (glob.glob results, os.walk dirnames, iterdir children, os.listdir). +* Remediation template context is not wall-clock dependent beyond the + YEAR field (which LICENSE templates require); DATE was dropped. +* List-valued template context is sorted before Jinja2 string join. +* Filesystem writes from the file_create and yaml_inject handlers are + atomic (tempfile-then-rename), leaving no partial file on crash. +""" + +from __future__ import annotations + +import os +from datetime import datetime +from pathlib import Path +from unittest.mock import patch + +import pytest + +from darnit.sieve.builtin_handlers import ( + _atomic_write_text, + _walk_depth_limited, + file_create_handler, +) +from darnit.sieve.handler_registry import HandlerContext, HandlerResultStatus + + +def _mk_ctx(local_path: Path) -> HandlerContext: + return HandlerContext( + local_path=str(local_path), + owner="", + repo="", + default_branch="main", + control_id="TEST", + project_context={}, + gathered_evidence={}, + shared_cache={}, + dependency_results={}, + ) + + +class TestAtomicWrite: + """_atomic_write_text: partial writes never leak to the target path.""" + + @pytest.mark.unit + def test_writes_content_atomically(self, tmp_path: Path) -> None: + target = tmp_path / "out.txt" + _atomic_write_text(str(target), "hello\n") + assert target.read_text() == "hello\n" + + @pytest.mark.unit + def test_no_partial_file_on_write_failure(self, tmp_path: Path) -> None: + """If write raises mid-flight, target is absent and no .tmp remains.""" + target = tmp_path / "out.txt" + + # Force os.replace to blow up after the tempfile is written. The + # atomic helper catches the exception, cleans the tempfile, and + # re-raises -- the target must NOT exist. + with patch("darnit.sieve.builtin_handlers.os.replace", side_effect=OSError("boom")): + with pytest.raises(OSError): + _atomic_write_text(str(target), "hello\n") + + assert not target.exists(), "target file must not exist after failed atomic write" + leftover = [p for p in tmp_path.iterdir() if p.name.startswith(".darnit-write-")] + assert leftover == [], f"tempfile leaked: {leftover}" + + +class TestFileCreateHandlerAtomic: + """file_create_handler goes through _atomic_write_text.""" + + @pytest.mark.unit + def test_file_create_leaves_no_partial_on_failure(self, tmp_path: Path) -> None: + target = tmp_path / "SECURITY.md" + with patch("darnit.sieve.builtin_handlers.os.replace", side_effect=OSError("disk full")): + result = file_create_handler( + {"handler": "file_create", "path": str(target), "content": "# Security\n"}, + _mk_ctx(tmp_path), + ) + + # Handler catches OSError and returns ERROR status; the point of + # this test is the *file* not the return value -- no partial file. + assert result.status == HandlerResultStatus.ERROR + assert not target.exists() + + +class TestWalkDepthLimitedSorted: + """_walk_depth_limited yields subdirs in sorted order.""" + + @pytest.mark.unit + def test_dirnames_visited_in_sorted_order(self, tmp_path: Path) -> None: + # Create subdirs in a jumbled order; os.walk's raw ordering is + # filesystem-dependent, but our helper must sort. + for name in ["z_alpha", "b_beta", "m_middle", "a_first"]: + (tmp_path / name).mkdir() + + visited: list[str] = [] + for dirpath, _depth in _walk_depth_limited(str(tmp_path), max_depth=1): + rel = os.path.relpath(dirpath, tmp_path) + if rel != ".": + visited.append(rel) + + assert visited == ["a_first", "b_beta", "m_middle", "z_alpha"] + + +class TestGlobSortedInFileExists: + """file_exists's glob-branch takes matches[0] in sorted order.""" + + @pytest.mark.unit + def test_glob_first_match_is_lexicographically_smallest(self, tmp_path: Path) -> None: + from darnit.sieve.builtin_handlers import file_exists_handler + + # Create three matches; the "first" one must be alphabetical, not + # filesystem-order-dependent. + for name in ["release-beta.yml", "release-alpha.yml", "release-gamma.yml"]: + (tmp_path / name).write_text("") + + result = file_exists_handler( + {"handler": "file_exists", "files": ["release-*.yml"]}, + _mk_ctx(tmp_path), + ) + + assert result.status == HandlerResultStatus.PASS + assert result.evidence["relative_path"] == "release-alpha.yml" + + +class TestExecutorTemplateContext: + """RemediationExecutor.now_provider + list-sort + DATE removed.""" + + def _executor(self, tmp_path: Path, **kwargs): + from darnit.remediation.executor import RemediationExecutor + + return RemediationExecutor(local_path=str(tmp_path), **kwargs) + + @pytest.mark.unit + def test_year_is_derived_from_now_provider(self, tmp_path: Path) -> None: + fixed = datetime(2030, 6, 15, 12, 0, 0) + ex = self._executor(tmp_path, now_provider=lambda: fixed) + ctx = ex._get_template_context("TEST-01") + assert ctx["YEAR"] == "2030" + + @pytest.mark.unit + def test_date_field_no_longer_present(self, tmp_path: Path) -> None: + """DATE was dropped -- see _get_template_context comment.""" + ex = self._executor(tmp_path) + ctx = ex._get_template_context("TEST-01") + assert "DATE" not in ctx + + @pytest.mark.unit + def test_year_is_stable_across_calls_with_fixed_now(self, tmp_path: Path) -> None: + fixed = datetime(2029, 3, 4) + ex = self._executor(tmp_path, now_provider=lambda: fixed) + first = ex._get_template_context("TEST-01")["YEAR"] + second = ex._get_template_context("TEST-01")["YEAR"] + assert first == second == "2029" + + @pytest.mark.unit + def test_list_context_values_are_sorted_before_join(self, tmp_path: Path) -> None: + """Different upstream orderings of the same set produce identical output.""" + set_1 = ["@charlie", "@alice", "@bob"] + set_2 = ["@bob", "@charlie", "@alice"] + + ex1 = self._executor(tmp_path, context_values={"maintainers": set_1}) + ex2 = self._executor(tmp_path, context_values={"maintainers": set_2}) + + ctx1 = ex1._get_template_context("TEST-01") + ctx2 = ex2._get_template_context("TEST-01") + + assert ctx1["context"]["maintainers"] == ctx2["context"]["maintainers"] + assert ctx1["context"]["maintainers"] == "@alice @bob @charlie" + + +class TestAutoDetectSortedIteration: + """context/auto_detect: sorted iteration in the two survey-flagged spots.""" + + @pytest.mark.unit + def test_detect_ci_provider_uses_sorted_listdir(self, tmp_path: Path, monkeypatch) -> None: + """Regression guard: os.listdir call is wrapped in sorted().""" + from darnit.context import auto_detect + + # Layout a repo that has .github/workflows with a mix of file names. + workflows = tmp_path / ".github" / "workflows" + workflows.mkdir(parents=True) + (workflows / "z_last.yml").write_text("") + (workflows / "a_first.yml").write_text("") + + # Spy on os.listdir. detect_ci_provider must consult sorted() over + # the raw listdir result. + seen_iterations: list[list[str]] = [] + real_listdir = os.listdir + + def spy_listdir(path): + entries = real_listdir(path) + seen_iterations.append(list(entries)) + return entries + + monkeypatch.setattr(auto_detect.os, "listdir", spy_listdir) + + provider = auto_detect.detect_ci_provider(str(tmp_path)) + assert provider == "github" + # There was at least one listdir over the workflows dir. + wf_iters = [i for i in seen_iterations if "z_last.yml" in i and "a_first.yml" in i] + assert wf_iters, "expected a listdir over .github/workflows" + # The listdir result was consumed via sorted() -- we can only + # observe the raw listdir shape, but any(...) evaluated after + # sorted must return the same bool. This test proves the sort + # doesn't break detection when the FS order is jumbled.