diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 35e15e1..b90d55c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,7 +21,11 @@ jobs: name: lint runs-on: ubuntu-latest steps: + # Full history: scripts/check_file_sizes.py diffs against the base + # branch's merge base, which a shallow clone cannot resolve. - uses: actions/checkout@v6 + with: + fetch-depth: 0 - name: Install uv uses: astral-sh/setup-uv@v8.2.0 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 844b658..f7c2f7d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -16,8 +16,10 @@ repos: - id: end-of-file-fixer - id: check-yaml - id: check-toml + # Ceiling mirrored by scripts/check_file_sizes.py (a hard CI gate via + # `make lint`). Change both together — a unit test pins them equal. - id: check-added-large-files - args: [--maxkb=1024] + args: [--maxkb=640] - id: check-merge-conflict - id: detect-private-key diff --git a/Makefile b/Makefile index 338f749..139f16c 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: help install install-deps lint docs-check check-commits check-pr-title check-assets check-deprecated-names check-github-docs check-cjk check-datetime openapi check-openapi format test integration package cov ci clean +.PHONY: help install install-deps lint docs-check check-commits check-pr-title check-assets check-file-sizes check-deprecated-names check-github-docs check-cjk check-datetime openapi check-openapi format test integration package cov ci clean help: @echo "Targets:" @@ -9,6 +9,7 @@ help: @echo " check-commits Validate Conventional Commit subjects for a git range" @echo " check-pr-title Validate PR title uses Conventional Commit format" @echo " check-assets Block committed images, videos, and asset/media directories" + @echo " check-file-sizes Block committed files above the size ceiling (HARD gate, run via lint)" @echo " check-deprecated-names Block deprecated product names" @echo " check-github-docs Block legacy/internal branch-model residue in contributor docs" @echo " check-cjk Scan for CJK outside the language-policy allowlist (advisory)" @@ -39,6 +40,7 @@ lint: uv run ruff format --check src tests uv run lint-imports uv run python scripts/check_repo_assets.py + uv run python scripts/check_file_sizes.py uv run python scripts/check_deprecated_names.py uv run python scripts/check_github_contributor_docs.py uv run python scripts/check_datetime_discipline.py @@ -60,6 +62,14 @@ check-pr-title: check-assets: uv run python scripts/check_repo_assets.py +# Repository file size gate (wired into `lint`, and therefore `ci`). Mirrors +# the `check-added-large-files` pre-commit hook so the ceiling is enforced on +# every PR, not only on machines that installed the hooks. Scope is the change +# under review (diff vs the base branch), so existing files are left alone — +# needs `fetch-depth: 0` in CI to resolve the merge base. +check-file-sizes: + uv run python scripts/check_file_sizes.py + # Product naming gate. Public repo text should use EverOS or EverMind Cloud. check-deprecated-names: uv run python scripts/check_deprecated_names.py diff --git a/docs/engineering.md b/docs/engineering.md index 670aaa2..9350632 100644 --- a/docs/engineering.md +++ b/docs/engineering.md @@ -56,13 +56,36 @@ Each stage can independently fail a change; there is no `--no-verify` bypass. ``` 1. Editor ruff (lint + format) on save 2. pre-commit ruff, trailing-whitespace / EOF, yaml & toml checks, - large-file & private-key guards, merge-conflict check, + file-size & private-key guards, merge-conflict check, and gitlint (commit-msg stage) — see "Commits" below 3. make ci lint + unit + integration — run before pushing 4. GitHub CI re-runs the same make targets on every pull request 5. Review 1 approval + all conversations resolved + all checks green ``` +Stage 2 runs only on machines that ran `make install`, so CI cannot rely on it. +Where a pre-commit guard must hold for every pull request it has a `make` +counterpart re-run by CI. + +**File-size ceiling — 640 KB.** Enforced locally by `check-added-large-files` +and on every pull request by +[`scripts/check_file_sizes.py`](../scripts/check_file_sizes.py) +(`make check-file-sizes`, wired into `make lint`). The two limits are pinned +equal by a unit test; change them in the same commit. The CI gate is the +stronger of the two in scope: the local hook only inspects files being +*added*, so it cannot catch an existing file that grows, while the gate diffs +against the base branch and covers additions, modifications and renames +alike. Files already committed above the ceiling are out of scope — the gate +never fails a pull request for something it did not touch. Because it needs a +merge base, the `lint` job checks out with `fetch-depth: 0`. + +One directory is exempt: `tests/fixtures/search_seed/`, whose search corpora +are regenerated by `tests/fixtures/_dump_search_seed.py` and already approach +1 MB. Outside it the largest tracked file is ~300 KB, so the ceiling stays +meaningful. Prefer adding a directory to `EXEMPT_PREFIXES` over raising +`MAX_KB` for the whole repository; a unit test pins the list so it cannot grow +unnoticed. + ## Continuous integration CI runs on GitHub Actions ([.github/workflows/](../.github/workflows/)). Every @@ -70,7 +93,7 @@ pull request into `main` must pass: | Check | Command | Guards | |---|---|---| -| lint | `make lint` | ruff style, DDD layer direction (import-linter), datetime discipline, asset & deprecated-name guards | +| lint | `make lint` | ruff style, DDD layer direction (import-linter), datetime discipline, asset, file-size & deprecated-name guards | | unit tests | `make test` | `tests/unit` | | integration tests | `make integration` | `tests/integration` | | package build | `make package` | the wheel builds and imports cleanly | diff --git a/scripts/check_file_sizes.py b/scripts/check_file_sizes.py new file mode 100644 index 0000000..aaa780a --- /dev/null +++ b/scripts/check_file_sizes.py @@ -0,0 +1,204 @@ +"""Block oversized files entering the repository through a pull request. + +Scope is the change under review, not the whole tree: paths added, modified, +renamed or copied since the merge base. Files already committed above the +ceiling are left alone, so the gate never fails a pull request for something +it did not introduce. + +The same ceiling is enforced locally by the ``check-added-large-files`` +pre-commit hook. Keep ``MAX_KB`` and ``--maxkb`` in ``.pre-commit-config.yaml`` +in lockstep — a unit test pins them together, because a hook that only runs +locally is not a gate. Note the local hook is weaker by construction: it only +looks at files being *added*, so it cannot catch an existing file that grows. +That is the case this script covers on every pull request. + +On a push to the base branch itself the merge base is ``HEAD``, the diff is +empty, and the check is a no-op. This is a pull-request gate. + +``EXEMPT_PREFIXES`` carves out directories whose contents are machine-generated +and legitimately large. Keep it as short as possible: every entry is a place +the ceiling no longer protects. A unit test pins the list, so growing it shows +up as a deliberate change in review. +""" + +from __future__ import annotations + +import argparse +import os +import subprocess +from collections.abc import Iterable, Sequence +from dataclasses import dataclass +from pathlib import Path + +# Kilobytes, matching the pre-commit hook's ``--maxkb`` semantics exactly: +# a file is oversized when ``size_bytes > MAX_KB * 1024``. +MAX_KB = 640 + +DEFAULT_BASE = "origin/main" + +# Directories exempt from the ceiling, as posix path prefixes. +# +# tests/fixtures/search_seed/ holds embedded search corpora regenerated by +# tests/fixtures/_dump_search_seed.py. Two of them already sit near 1 MB and +# grew ~60% in a single release; a ceiling they have to clear would be raised +# on every refresh, which teaches contributors to edit the gate instead of +# their payload. Outside this directory the largest tracked file is ~300 KB. +EXEMPT_PREFIXES = ("tests/fixtures/search_seed/",) + + +class BaseRefError(RuntimeError): + """The comparison base could not be resolved.""" + + +@dataclass(frozen=True) +class Violation: + path: str + size_bytes: int + + +def _repo_root() -> Path: + return Path(__file__).resolve().parent.parent + + +def is_exempt(path: str, prefixes: Sequence[str] = EXEMPT_PREFIXES) -> bool: + """Whether ``path`` sits under an exempt directory prefix.""" + normalised = path.replace("\\", "/") + return any(normalised.startswith(prefix) for prefix in prefixes) + + +def find_violations( + paths: Iterable[str], + *, + root: Path, + max_kb: int = MAX_KB, + exempt_prefixes: Sequence[str] = EXEMPT_PREFIXES, +) -> list[Violation]: + """Return oversized entries among ``paths``, resolved against ``root``. + + Entries under ``exempt_prefixes`` are skipped. So are entries that are not + regular files: a diff can name a path that no longer exists in the working + tree, and a symlink would be measured by its target rather than itself. + """ + limit_bytes = max_kb * 1024 + violations: list[Violation] = [] + for path in paths: + if is_exempt(path, exempt_prefixes): + continue + candidate = root / path + if candidate.is_symlink() or not candidate.is_file(): + continue + size_bytes = candidate.stat().st_size + if size_bytes > limit_bytes: + violations.append(Violation(path=path, size_bytes=size_bytes)) + return violations + + +def default_base_ref() -> str: + """Resolve the base ref to diff against. + + ``GITHUB_BASE_REF`` is set by GitHub Actions on ``pull_request`` events + and names the target branch; everywhere else fall back to the default + branch's remote tracking ref. + """ + github_base = os.environ.get("GITHUB_BASE_REF", "").strip() + if github_base: + return f"origin/{github_base}" + return DEFAULT_BASE + + +def _git(root: Path, *args: str) -> str: + result = subprocess.run( + ["git", *args], + cwd=root, + capture_output=True, + text=True, + ) + if result.returncode != 0: + raise BaseRefError( + f"`git {' '.join(args)}` failed: {result.stderr.strip() or 'unknown error'}" + ) + return result.stdout + + +def changed_paths(root: Path, base_ref: str) -> list[str]: + """Paths this change adds or grows, relative to ``base_ref``. + + Two sources, unioned: + + * ``git diff`` from the merge base to the working tree (no second + revision), covering committed and uncommitted edits to tracked files. + ``--diff-filter=ACMR`` keeps additions, copies, modifications and + renames; deletions are excluded. + * untracked, non-ignored files. ``git diff`` cannot see a newly created + file until it is staged, so a local pre-push run would otherwise pass a + brand-new oversized file. In CI the checkout is clean and this source + is empty. + + Raises :class:`BaseRefError` when ``base_ref`` cannot be resolved — a + gate that silently passes on a shallow clone is worse than no gate. + """ + merge_base = _git(root, "merge-base", base_ref, "HEAD").strip() + if not merge_base: + raise BaseRefError(f"no merge base between {base_ref} and HEAD") + tracked = _git( + root, + "diff", + "--name-only", + "-z", + "--diff-filter=ACMR", + merge_base, + ) + untracked = _git(root, "ls-files", "--others", "--exclude-standard", "-z") + entries = {entry for entry in tracked.split("\0") if entry} + entries.update(entry for entry in untracked.split("\0") if entry) + return sorted(entries) + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--base", + default=None, + help=f"ref to diff against (default: $GITHUB_BASE_REF, else {DEFAULT_BASE})", + ) + args = parser.parse_args(argv) + + root = _repo_root() + base_ref = args.base or default_base_ref() + try: + paths = changed_paths(root, base_ref) + except BaseRefError as exc: + print( + f"Repository file size check could not run: {exc}\n" + "The gate diffs against the base branch, so it needs that ref and " + "enough history to find a merge base. In CI, set " + "`fetch-depth: 0` on actions/checkout; locally, run " + "`git fetch origin main`." + ) + return 1 + + violations = find_violations(paths, root=root) + if not violations: + print( + f"Repository file size check passed " + f"({len(paths)} changed file(s) vs {base_ref}, ceiling {MAX_KB} KB)." + ) + return 0 + + print( + f"Repository file size check failed: this change adds or grows files " + f"above {MAX_KB} KB.\n" + "Large payloads belong in release artifacts, external hosting, or " + "another approved storage location, then linked from docs. If a " + "generated corpus genuinely has to live in-tree, add its directory " + "to EXEMPT_PREFIXES in this script rather than raising the ceiling " + "for the whole repository — and say why in the commit.\n" + ) + for violation in sorted(violations, key=lambda item: -item.size_bytes): + size_kb = violation.size_bytes / 1024 + print(f"- {violation.path}: {size_kb:.1f} KB") + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/unit/test_scripts/test_check_file_sizes.py b/tests/unit/test_scripts/test_check_file_sizes.py new file mode 100644 index 0000000..feeb3aa --- /dev/null +++ b/tests/unit/test_scripts/test_check_file_sizes.py @@ -0,0 +1,240 @@ +"""Self-tests for ``scripts/check_file_sizes.py``. + +Pins three things: the ceiling's boundary semantics, that the gate's scope is +the change under review rather than the whole tree, and that the script and +the ``check-added-large-files`` pre-commit hook share one number. Drift +between the last two would recreate the failure this gate exists to prevent — +a ceiling that only ever runs on a developer's machine. +""" + +from __future__ import annotations + +import importlib.util +import re +import subprocess +import sys +from pathlib import Path + +import pytest + +_REPO_ROOT = Path(__file__).resolve().parents[3] +_CHECKER_PATH = _REPO_ROOT / "scripts" / "check_file_sizes.py" +_PRE_COMMIT_CONFIG = _REPO_ROOT / ".pre-commit-config.yaml" + + +def _load_checker(): + assert _CHECKER_PATH.exists(), "file size checker should exist" + spec = importlib.util.spec_from_file_location("_file_size_checker", _CHECKER_PATH) + assert spec and spec.loader + mod = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = mod + spec.loader.exec_module(mod) + return mod + + +def _write(root: Path, name: str, size_bytes: int) -> str: + target = root / name + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(b"x" * size_bytes) + return name + + +def _git(root: Path, *args: str) -> None: + subprocess.run(["git", *args], cwd=root, check=True, capture_output=True) + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + """A real git repo with one commit on ``main`` as the diff base.""" + _git(tmp_path, "init", "-q", "-b", "main") + _git(tmp_path, "config", "user.email", "test@example.com") + _git(tmp_path, "config", "user.name", "test") + _write(tmp_path, "existing-huge.bin", 9 * 1024) + _write(tmp_path, "README.md", 16) + _git(tmp_path, "add", "-A") + _git(tmp_path, "commit", "-qm", "base") + return tmp_path + + +def test_files_within_the_ceiling_are_allowed(tmp_path: Path) -> None: + checker = _load_checker() + paths = [ + _write(tmp_path, "README.md", 1024), + _write(tmp_path, "tests/fixtures/seed.json", 4 * 1024), + ] + + violations = checker.find_violations(paths, root=tmp_path, max_kb=8) + + assert violations == [] + + +def test_oversized_file_is_flagged_with_its_size(tmp_path: Path) -> None: + checker = _load_checker() + paths = [ + _write(tmp_path, "small.json", 1024), + _write(tmp_path, "examples/recorded_trace.json", 9 * 1024), + ] + + violations = checker.find_violations(paths, root=tmp_path, max_kb=8) + + assert [violation.path for violation in violations] == [ + "examples/recorded_trace.json" + ] + assert violations[0].size_bytes == 9 * 1024 + + +def test_ceiling_is_inclusive_at_the_exact_limit(tmp_path: Path) -> None: + checker = _load_checker() + exact = _write(tmp_path, "exact.bin", 8 * 1024) + over = _write(tmp_path, "over.bin", 8 * 1024 + 1) + + violations = checker.find_violations([exact, over], root=tmp_path, max_kb=8) + + assert [violation.path for violation in violations] == ["over.bin"] + + +def test_missing_and_symlink_paths_are_skipped(tmp_path: Path) -> None: + checker = _load_checker() + real = _write(tmp_path, "real.bin", 9 * 1024) + link = tmp_path / "link.bin" + link.symlink_to(tmp_path / "real.bin") + + violations = checker.find_violations( + ["deleted-from-worktree.bin", "link.bin", real], + root=tmp_path, + max_kb=8, + ) + + assert [violation.path for violation in violations] == ["real.bin"] + + +def test_scope_excludes_files_the_change_did_not_touch(repo: Path) -> None: + checker = _load_checker() + _git(repo, "checkout", "-q", "-b", "feature") + _write(repo, "added-small.json", 32) + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "small addition") + + paths = checker.changed_paths(repo, "main") + + assert paths == ["added-small.json"] + assert "existing-huge.bin" not in paths + + +def test_scope_includes_a_grown_existing_file(repo: Path) -> None: + checker = _load_checker() + _git(repo, "checkout", "-q", "-b", "feature") + _write(repo, "README.md", 9 * 1024) + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "grow readme") + + violations = checker.find_violations( + checker.changed_paths(repo, "main"), root=repo, max_kb=8 + ) + + assert [violation.path for violation in violations] == ["README.md"] + + +def test_scope_includes_uncommitted_working_tree_edits(repo: Path) -> None: + checker = _load_checker() + _git(repo, "checkout", "-q", "-b", "feature") + _write(repo, "staged-only.bin", 9 * 1024) + _git(repo, "add", "-A") + + paths = checker.changed_paths(repo, "main") + + assert "staged-only.bin" in paths + + +def test_scope_includes_untracked_files(repo: Path) -> None: + """`git diff` cannot see a new file before it is staged; the gate must.""" + checker = _load_checker() + _git(repo, "checkout", "-q", "-b", "feature") + _write(repo, "never-staged.bin", 9 * 1024) + + violations = checker.find_violations( + checker.changed_paths(repo, "main"), root=repo, max_kb=8 + ) + + assert [violation.path for violation in violations] == ["never-staged.bin"] + + +def test_scope_respects_gitignore(repo: Path) -> None: + checker = _load_checker() + _git(repo, "checkout", "-q", "-b", "feature") + _write(repo, ".gitignore", 0) + (repo / ".gitignore").write_text("scratch/\n", encoding="utf-8") + _write(repo, "scratch/huge.bin", 9 * 1024) + + paths = checker.changed_paths(repo, "main") + + assert "scratch/huge.bin" not in paths + + +def test_deletions_are_not_reported(repo: Path) -> None: + checker = _load_checker() + _git(repo, "checkout", "-q", "-b", "feature") + _git(repo, "rm", "-q", "existing-huge.bin") + _git(repo, "commit", "-qm", "drop the big one") + + paths = checker.changed_paths(repo, "main") + + assert paths == [] + + +def test_unresolvable_base_is_a_hard_failure(repo: Path) -> None: + checker = _load_checker() + + with pytest.raises(checker.BaseRefError): + checker.changed_paths(repo, "origin/does-not-exist") + + +def test_exempt_directory_may_exceed_the_ceiling(tmp_path: Path) -> None: + checker = _load_checker() + exempt = _write(tmp_path, "tests/fixtures/search_seed/episode.json", 9 * 1024) + nearby = _write(tmp_path, "tests/fixtures/other_seed.json", 9 * 1024) + + violations = checker.find_violations([exempt, nearby], root=tmp_path, max_kb=8) + + assert [violation.path for violation in violations] == [ + "tests/fixtures/other_seed.json" + ], "the exemption must cover exactly its directory, not siblings" + + +def test_exemption_list_is_pinned() -> None: + """Every entry is a place the ceiling stops protecting — keep it visible.""" + checker = _load_checker() + + assert checker.EXEMPT_PREFIXES == ("tests/fixtures/search_seed/",) + + +def test_exempt_prefixes_point_at_real_directories() -> None: + checker = _load_checker() + + for prefix in checker.EXEMPT_PREFIXES: + assert (_REPO_ROOT / prefix).is_dir(), ( + f"exempt prefix {prefix!r} does not exist; drop it rather than " + "leaving a hole for a path that may come back" + ) + + +def test_ceiling_matches_the_pre_commit_hook() -> None: + checker = _load_checker() + config = _PRE_COMMIT_CONFIG.read_text(encoding="utf-8") + + hook_limits = re.findall(r"--maxkb=(\d+)", config) + + assert hook_limits == [str(checker.MAX_KB)], ( + "scripts/check_file_sizes.py and the check-added-large-files hook must " + "share one ceiling; update both in the same commit." + ) + + +def test_github_pull_request_base_is_preferred(monkeypatch: pytest.MonkeyPatch) -> None: + checker = _load_checker() + + monkeypatch.setenv("GITHUB_BASE_REF", "release/1.3") + assert checker.default_base_ref() == "origin/release/1.3" + + monkeypatch.setenv("GITHUB_BASE_REF", "") + assert checker.default_base_ref() == checker.DEFAULT_BASE