From fafb828bb767b89d1f6ee5b6ce14350503f68cfd Mon Sep 17 00:00:00 2001 From: codex Date: Sun, 26 Jul 2026 07:50:44 +0000 Subject: [PATCH] ci(coverage): enforce the critical core contract --- .gitea/workflows/update-badges.yml | 21 +++-- docs/decisions/0004-coverage-policy.md | 16 ++-- scripts/coverage.sh | 16 ++-- scripts/critical-modules.txt | 47 +++++++++--- scripts/critical_modules.py | 101 +++++++++++++++++++++++++ tests/unit/test_critical_modules.py | 54 +++++++++++++ 6 files changed, 226 insertions(+), 29 deletions(-) create mode 100644 scripts/critical_modules.py create mode 100644 tests/unit/test_critical_modules.py diff --git a/.gitea/workflows/update-badges.yml b/.gitea/workflows/update-badges.yml index 94ec1a98..223de9de 100644 --- a/.gitea/workflows/update-badges.yml +++ b/.gitea/workflows/update-badges.yml @@ -33,19 +33,28 @@ jobs: - name: Run coverage and extract percentage id: coverage run: | - python3 -m coverage run -m unittest discover -t . -s tests/unit > /dev/null 2>&1 || true - PERCENT=$(python3 -m coverage report 2>/dev/null | grep '^TOTAL' | grep -oP '\d+(?=%)' | tail -1) + set -euo pipefail + # Never publish a badge from a failed or partial test run. + python3 -m coverage run -m unittest discover -t . -s tests/unit + REPORT=$(python3 -m coverage report) + printf '%s\n' "$REPORT" + PERCENT=$(printf '%s\n' "$REPORT" | awk '$1 == "TOTAL" {gsub("%", "", $NF); print $NF}') + test -n "$PERCENT" echo "percent=$PERCENT" >> $GITHUB_OUTPUT echo "Coverage: $PERCENT%" - name: Extract core (critical-module) coverage percentage id: core_coverage run: | + set -euo pipefail # Reuses the .coverage data from the previous step. The core list is - # the single source of truth in scripts/critical-modules.txt; every - # core module is unit-tested, so the unit-only run is accurate for it. - INCLUDE=$(grep -vE '^[[:space:]]*(#|$)' scripts/critical-modules.txt | paste -sd, -) - PERCENT=$(python3 -m coverage report --include="$INCLUDE" 2>/dev/null | grep '^TOTAL' | grep -oP '\d+(?=%)' | tail -1) + # validated single source of truth. Fail if a listed path disappeared + # or if the measured core falls below ADR 0004's 90% minimum. + INCLUDE=$(python3 scripts/critical_modules.py) + REPORT=$(python3 -m coverage report --include="$INCLUDE" --fail-under=90) + printf '%s\n' "$REPORT" + PERCENT=$(printf '%s\n' "$REPORT" | awk '$1 == "TOTAL" {gsub("%", "", $NF); print $NF}') + test -n "$PERCENT" echo "percent=$PERCENT" >> $GITHUB_OUTPUT echo "Core coverage: $PERCENT%" diff --git a/docs/decisions/0004-coverage-policy.md b/docs/decisions/0004-coverage-policy.md index acaaa4e2..cb0ff858 100644 --- a/docs/decisions/0004-coverage-policy.md +++ b/docs/decisions/0004-coverage-policy.md @@ -34,12 +34,13 @@ a regression (Goodhart's law). Coverage is **risk-weighted**, measured over the **combined unit + integration** suites, with three rules: -1. **Critical modules target ≥ 90%.** The security/logic core — - `egress_addon{,_core}.py`, `dlp_detectors.py`, `egress.py`, - `manifest*.py`, `git_gate.py`, `git_http_backend.py`, `supervise.py`, - `yaml_subset.py`, `bottle_state.py` — is Docker-independent and - unit-testable, so it carries the high bar. We ratchet toward 90% as - these modules are touched; new gaps in them are not acceptable. +1. **Critical modules must remain ≥ 90%.** The curated security/logic core + covers the host and gateway egress policy, manifest trust boundary, + git-gate enforcement, supervise protocol/server, YAML parser, and bottle + state. The concrete module list lives in `scripts/critical-modules.txt`; + `scripts/critical_modules.py` rejects stale or ambiguous entries before + Coverage.py can silently ignore them. These modules are unit-testable, so + CI enforces the aggregate minimum independently of diff coverage. 2. **Subprocess/backend orchestration is covered by the integration suite, not omitted.** `scripts/coverage.sh` runs unit + integration @@ -82,6 +83,9 @@ omit list. (critical-module standard + diff coverage) are Docker-independent. - "We're at N%" is now a curated figure; outsiders should read the policy, not just the badge. +- A rename or removal in the curated list fails CI. Updating the list is an + explicit review of where the security-critical behavior moved, not a way to + improve the percentage by omission. ## Links diff --git a/scripts/coverage.sh b/scripts/coverage.sh index b202cc70..0dab9a84 100755 --- a/scripts/coverage.sh +++ b/scripts/coverage.sh @@ -20,10 +20,10 @@ cd "$(dirname "$0")/.." PY="${PYTHON:-python3}" -# Critical security/logic core held to the high bar by ADR 0004. The list -# lives in one place (scripts/critical-modules.txt) so this report and the -# README "core coverage" badge can't drift; comma-join it for --include. -CRITICAL=$(grep -vE '^[[:space:]]*(#|$)' scripts/critical-modules.txt | paste -sd, -) +# Critical security/logic core held to the high bar by ADR 0004. The helper +# fails before coverage when a curated path was renamed or removed; Coverage.py +# itself would silently ignore that stale include and inflate the score. +CRITICAL=$("$PY" scripts/critical_modules.py) if [ "${1:-}" = "aggregate" ]; then # Aggregate mode: combine .coverage.* artifacts already in the workspace. @@ -34,8 +34,8 @@ if [ "${1:-}" = "aggregate" ]; then "$PY" -m coverage report -m if [ "${2:-}" = "critical" ]; then - echo "== critical modules (ADR 0004 target: 90%) ==" >&2 - "$PY" -m coverage report --include="$CRITICAL" + echo "== critical modules (ADR 0004 minimum: 90%) ==" >&2 + "$PY" -m coverage report --include="$CRITICAL" --fail-under=90 fi exit 0 fi @@ -55,6 +55,6 @@ echo "== combined report ==" >&2 "$PY" -m coverage report -m if [ "${1:-}" = "critical" ]; then - echo "== critical modules (ADR 0004 target: 90%) ==" >&2 - "$PY" -m coverage report --include="$CRITICAL" + echo "== critical modules (ADR 0004 minimum: 90%) ==" >&2 + "$PY" -m coverage report --include="$CRITICAL" --fail-under=90 fi diff --git a/scripts/critical-modules.txt b/scripts/critical-modules.txt index 845e4154..1da2dbd0 100644 --- a/scripts/critical-modules.txt +++ b/scripts/critical-modules.txt @@ -7,19 +7,48 @@ # number that silently stops measuring a module is worse than no badge. # # One module path per line, relative to the repo root. Blank lines and -# `#` comments are ignored. +# `#` comments are ignored. scripts/critical_modules.py rejects missing, +# duplicate, non-Python, and out-of-repository entries before coverage runs. + +# Host-side egress planning and secret preparation. +bot_bottle/egress/plan.py +bot_bottle/egress/service.py + +# Gateway egress policy, matching, and DLP enforcement. bot_bottle/gateway/egress/addon.py bot_bottle/gateway/egress/addon_core.py +bot_bottle/gateway/egress/context.py +bot_bottle/gateway/egress/dlp.py +bot_bottle/gateway/egress/dlp_config.py bot_bottle/gateway/egress/dlp_detectors.py -bot_bottle/egress.py -bot_bottle/manifest.py -bot_bottle/manifest_egress.py -bot_bottle/manifest_agent.py -bot_bottle/manifest_schema.py -bot_bottle/git_gate.py +bot_bottle/gateway/egress/matching.py +bot_bottle/gateway/egress/schema.py +bot_bottle/gateway/egress/types.py + +# Manifest trust boundary and schema. +bot_bottle/manifest/agent.py +bot_bottle/manifest/bottle.py +bot_bottle/manifest/egress.py +bot_bottle/manifest/extends.py +bot_bottle/manifest/git.py +bot_bottle/manifest/index.py +bot_bottle/manifest/loader.py +bot_bottle/manifest/schema.py +bot_bottle/manifest/util.py + +# Host-side and gateway-side git policy enforcement. +bot_bottle/git_gate/host_key.py +bot_bottle/git_gate/plan.py +bot_bottle/git_gate/provision.py +bot_bottle/git_gate/service.py bot_bottle/gateway/git_gate/render.py -bot_bottle/git_gate_provision.py bot_bottle/gateway/git_gate/http_backend.py -bot_bottle/supervise.py + +# Supervise proposal protocol and data plane. +bot_bottle/supervisor/plan.py +bot_bottle/supervisor/types.py +bot_bottle/gateway/supervisor/server.py + +# Shared parsers and state validation. bot_bottle/yaml_subset.py bot_bottle/bottle_state.py diff --git a/scripts/critical_modules.py b/scripts/critical_modules.py new file mode 100644 index 00000000..c6ad3b4a --- /dev/null +++ b/scripts/critical_modules.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python3 +"""Validate and render the critical-module coverage manifest. + +Coverage.py silently ignores an ``--include`` path that does not exist. That +is useful for broad globs, but dangerous for bot-bottle's curated security +core: a rename could otherwise improve the reported percentage by removing a +module from the measurement. Keep the validation in one small stdlib helper +and make every coverage consumer call it. +""" + +from __future__ import annotations + +import argparse +import sys +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parents[1] +DEFAULT_MANIFEST = REPO_ROOT / "scripts" / "critical-modules.txt" + + +class CriticalModulesError(ValueError): + """The critical-module manifest is empty, ambiguous, or stale.""" + + +def load_critical_modules(manifest: Path, *, root: Path) -> list[str]: + """Return validated module paths relative to *root*. + + Entries must be unique, concrete Python files inside the repository. + Globs are deliberately rejected by the file check: each rename must update + this explicit security review surface. + """ + + root = root.resolve() + try: + lines = manifest.read_text(encoding="utf-8").splitlines() + except OSError as exc: + raise CriticalModulesError( + f"cannot read critical-module manifest {manifest}: {exc}" + ) from exc + + modules: list[str] = [] + seen: set[str] = set() + errors: list[str] = [] + for line_number, raw in enumerate(lines, start=1): + entry = raw.strip() + if not entry or entry.startswith("#"): + continue + path = Path(entry) + prefix = f"{manifest}:{line_number}: {entry!r}" + if path.is_absolute(): + errors.append(f"{prefix} must be relative to the repository root") + continue + try: + resolved = (root / path).resolve() + resolved.relative_to(root) + except ValueError: + errors.append(f"{prefix} escapes the repository root") + continue + if entry in seen: + errors.append(f"{prefix} is duplicated") + continue + seen.add(entry) + if path.suffix != ".py": + errors.append(f"{prefix} is not a Python module") + continue + if not resolved.is_file(): + errors.append(f"{prefix} does not exist") + continue + modules.append(path.as_posix()) + + if not modules and not errors: + errors.append(f"{manifest}: contains no critical modules") + if errors: + raise CriticalModulesError("\n".join(errors)) + return modules + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description="validate and print the critical coverage include list" + ) + parser.add_argument("--manifest", type=Path, default=DEFAULT_MANIFEST) + parser.add_argument("--root", type=Path, default=REPO_ROOT) + parser.add_argument( + "--check", action="store_true", + help="validate only; do not print the comma-separated include list", + ) + args = parser.parse_args(argv) + try: + modules = load_critical_modules(args.manifest, root=args.root) + except CriticalModulesError as exc: + print(f"critical-modules: {exc}", file=sys.stderr) + return 1 + if not args.check: + print(",".join(modules)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/unit/test_critical_modules.py b/tests/unit/test_critical_modules.py new file mode 100644 index 00000000..a2bc9098 --- /dev/null +++ b/tests/unit/test_critical_modules.py @@ -0,0 +1,54 @@ +"""Tests for the fail-closed critical coverage manifest.""" + +from __future__ import annotations + +import tempfile +import unittest +from pathlib import Path + +from scripts.critical_modules import ( + DEFAULT_MANIFEST, + REPO_ROOT, + CriticalModulesError, + load_critical_modules, +) + + +class TestCriticalModules(unittest.TestCase): + def test_repository_manifest_is_valid(self) -> None: + modules = load_critical_modules(DEFAULT_MANIFEST, root=REPO_ROOT) + self.assertGreater(len(modules), 20) + self.assertEqual(len(modules), len(set(modules))) + + def test_missing_module_fails(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + manifest = root / "critical-modules.txt" + manifest.write_text("bot_bottle/renamed.py\n", encoding="utf-8") + with self.assertRaisesRegex(CriticalModulesError, "does not exist"): + load_critical_modules(manifest, root=root) + + def test_duplicate_module_fails(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + module = root / "bot_bottle" / "core.py" + module.parent.mkdir() + module.write_text("", encoding="utf-8") + manifest = root / "critical-modules.txt" + manifest.write_text( + "bot_bottle/core.py\nbot_bottle/core.py\n", encoding="utf-8" + ) + with self.assertRaisesRegex(CriticalModulesError, "duplicated"): + load_critical_modules(manifest, root=root) + + def test_entry_cannot_escape_repository(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + manifest = root / "critical-modules.txt" + manifest.write_text("../outside.py\n", encoding="utf-8") + with self.assertRaisesRegex(CriticalModulesError, "escapes"): + load_critical_modules(manifest, root=root) + + +if __name__ == "__main__": + unittest.main()