diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2f8d789..88718b5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -51,6 +51,7 @@ jobs: pip install . stevedore-release-scope --help > /dev/null service-yaml-check > /dev/null + no-production-newtonsoft --help > /dev/null - name: try-repo each hook against a fixture consumer run: | @@ -76,10 +77,16 @@ jobs: image: pinpredict/oms YAML + # A tracked, clean production source so no-production-newtonsoft has + # something to actually scan — with zero matching files it would pass + # without exercising the git enumeration path. + mkdir -p "$fixture/Acme.Api" + printf 'using System.Text.Json;\n' > "$fixture/Acme.Api/Program.cs" + git -c init.defaultBranch=main init -q --template= "$fixture" git -C "$fixture" add -A - for hook in stevedore-release-scope service-yaml-check; do + for hook in stevedore-release-scope service-yaml-check no-production-newtonsoft; do echo "::group::try-repo $hook" (cd "$fixture" && pre-commit try-repo "$GITHUB_WORKSPACE" "$hook" --all-files) echo "::endgroup::" diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml index d37c9c7..8f81d9b 100644 --- a/.pre-commit-hooks.yaml +++ b/.pre-commit-hooks.yaml @@ -39,6 +39,22 @@ files: '^(\.stevedore\.yaml|\.github/workflows/ci\.yml|charts/[^/]+/Chart\.yaml)$' pass_filenames: false +- id: no-production-newtonsoft + name: reject production Newtonsoft references + description: | + Case-insensitive scan of every tracked .NET source and build file for a + Newtonsoft reference, allowed only under the path prefixes named by + --allow-prefix (the approved test and benchmark projects) and on the single + central PackageVersion line named by --central-version-file. This is the + static half of the policy; the transitive package-graph half needs `dotnet + restore` and private feed credentials, so it stays in the consumer's CI. + Enumerates files with `git ls-files`, not the staged set, so a violation in + a file this commit did not touch still fails. + entry: no-production-newtonsoft + language: python + pass_filenames: false + always_run: true + - id: check-go-version-sync name: check go version sync (go.mod ↔ .tool-versions) description: | diff --git a/README.md b/README.md index fcf4c1a..f1b2dff 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,7 @@ repos. | `csharpier-worktree-guard` | Run `dotnet csharpier format .` (or `check .` via `CSHARPIER_MODE=check`) but fail loudly if csharpier reports "0 files" while the repo actually contains tracked `.cs` files. Catches the silent no-op observed when csharpier runs from inside a git worktree. | `*.cs` | | `service-yaml-check` | Static checks for new/changed `.platform/services/.yaml` files: chart path resolves, ECR repo / PIA / GHA push-role exist in TF, networkPolicy ingress ports match the chart's declared health port. Catches the post-merge failure modes from [platform-gitops#544](https://github.com/pinpredict/platform-gitops/issues/544). | `.platform/services/*.yaml` | | `stevedore-release-scope` | Assert that onboarding or retiring a service does not widen the shared image build contract: named services pair an `.stevedore.yaml` image id with a name-matching sibling chart, `docker-release` and `chart-release` receive the same `only:` selector, and `change_detection.shared_paths` carries the all-image signal without listing paths every onboarding touches. | `.stevedore.yaml`, `.github/workflows/ci.yml`, `charts/*/Chart.yaml` | +| `no-production-newtonsoft` | Reject Newtonsoft.Json references in production .NET sources: a case-insensitive scan of every tracked source and build file, permitted only under the `--allow-prefix` paths (the approved test/benchmark projects) and on the one central `PackageVersion` line. Static half only — the transitive package-graph half needs `dotnet restore` and stays in the consumer's CI. | every commit (whole-tree scan) | | `check-go-version-sync` | Fails when a `go.mod` `go` directive and the governing `.tool-versions` `golang` pin drift apart. | `go.mod`, `.tool-versions` | ## Using a hook @@ -153,6 +154,51 @@ variant frozen; the docker/chart consistency check runs either way. All violations are collected and reported in one run rather than failing on the first, so a single `pre-commit run` shows the whole picture. +## `no-production-newtonsoft` + +Production .NET code should use `System.Text.Json`; a `Newtonsoft.Json` +reference is allowed only in explicitly approved test and benchmark projects. +Nothing is exempt by default — name every permitted prefix: + +```yaml +- repo: https://github.com/pinpredict/pre-commit-hooks + rev: v0.4.0 + hooks: + - id: no-production-newtonsoft + args: + - --allow-prefix=PinPredict.Tests/ + - --allow-prefix=PinPredict.ParlayManager.Tests/ + - --allow-prefix=PinPredict.Benchmarks/ + - --central-version-file=Directory.Packages.props +``` + +| Flag | Purpose | +|---|---| +| `--allow-prefix PREFIX` (repeatable) | path prefix where a reference is permitted; nothing is allowed by default | +| `--central-version-file PATH` | the one file permitted to declare the package's central `PackageVersion` | +| `--token TOKEN` | case-insensitive token that marks a violation (default `newtonsoft`) | +| `--package ID` | package id for the central-version exemption (default `.Json`) | +| `--source-glob GLOB` (repeatable) | git pathspecs to scan (default `*.cs *.csproj *.props *.targets`) | +| `--root PATH` | repository root to scan (default the working directory) | + +**This is the static half of the policy only.** Catching Newtonsoft that arrives +*transitively* needs `dotnet list package --include-transitive`, which needs +`dotnet restore` and the solution's private feed credentials — far too slow and +too credential-bound for a commit hook. Keep that guard in your own CI beside +the SDK install; this hook covers the direct references, which are the ones a +commit actually introduces. + +Two contract details worth knowing before you tune it: + +- **The scan is whole-tree, not staged-files.** Files come from `git ls-files` + and the hook runs `pass_filenames: false` / `always_run: true`. The policy is a + property of the repository, so a violation sitting in a file your commit never + touched must still fail — scoping to changed files would let a pre-existing + reference stay invisible forever. +- **The central-version exemption is scoped to one named file.** A + `PackageVersion` element copied into an ordinary project file is still a + violation, so the exemption can't be used to smuggle a reference in. + ## Why this repo exists Pre-commit hooks defined as `repo: local` in a `.pre-commit-config.yaml` are diff --git a/pinpredict_hooks/_common.py b/pinpredict_hooks/_common.py new file mode 100644 index 0000000..3f5ef76 --- /dev/null +++ b/pinpredict_hooks/_common.py @@ -0,0 +1,44 @@ +#!/usr/bin/env python3 +"""Helpers shared by the Python hooks in this repo. + +Private module — not exposed as a console script. Everything here is behavior +that every hook needs to get identically right: reading an optional YAML file, +and reporting failures so GitHub Actions annotates them while a local terminal +stays readable. +""" + +from __future__ import annotations + +import os +from pathlib import Path +from typing import Any + +import yaml + +# Distinguishes "the file is not there" from "the file parsed to None". A hook +# must treat an absent surface as a clean no-op but an empty one as a real +# (checkable) document — collapsing both to None loses that. +MISSING = object() + + +def load_yaml(path: Path, prog: str) -> Any: + """Parse `path`, returning MISSING when the file does not exist. + + A malformed document is fatal rather than a skipped check: silently passing + on unparseable YAML is how a guard stops guarding without anyone noticing. + """ + try: + text = path.read_text(encoding="utf-8") + except FileNotFoundError: + return MISSING + try: + return yaml.safe_load(text) + except yaml.YAMLError as error: + raise SystemExit(f"{prog}: {path} is not valid YAML: {error}") + + +def emit(failures: list[str], label: str) -> None: + """Print each failure, as a GHA annotation under Actions and plainly elsewhere.""" + prefix = "::error::" if os.environ.get("GITHUB_ACTIONS") == "true" else "ERROR: " + for failure in failures: + print(f"{prefix}{label}: {failure}") diff --git a/pinpredict_hooks/no_production_newtonsoft.py b/pinpredict_hooks/no_production_newtonsoft.py new file mode 100644 index 0000000..4616c3a --- /dev/null +++ b/pinpredict_hooks/no_production_newtonsoft.py @@ -0,0 +1,201 @@ +#!/usr/bin/env python3 +"""Reject Newtonsoft.Json references in production .NET sources. + +Production code should use `System.Text.Json`; a Newtonsoft reference is allowed +only in explicitly approved test and benchmark projects. This hook is the +**static** half of that policy: a case-insensitive scan of every tracked source +and build file, with no `dotnet` involvement, so it runs on every commit in well +under a second. + +The **transitive** half — resolving each project's package graph via +`dotnet list package --include-transitive` to catch Newtonsoft arriving through +a dependency — deliberately does *not* live here. It needs `dotnet restore`, +the solution's private package feeds, and credentials, so it belongs in CI +alongside the SDK install. Consumers keep that guard in their own repo; this +hook covers the direct references, which are the ones a commit introduces. + +Every repo-specific detail is a flag, so the hook carries no consumer's layout: + + * `--allow-prefix` — path prefixes where a reference is permitted (the test + and benchmark projects). Nothing is allowed by default. + * `--central-version-file` — a central package-management file permitted to + declare the `PackageVersion` for the forbidden package, since pinning a + version centrally is how the approved test projects consume it at all. + * `--source-glob` / `--token` — override the file set and the matched token. + +Files are enumerated with `git ls-files` rather than from pre-commit's staged +filenames, and the hook runs with `pass_filenames: false`. That is deliberate: +the policy is a property of the whole tree, so a violation sitting in a file +this commit did not touch must still fail. Scanning only changed files would +let a pre-existing reference stay invisible forever. +""" + +from __future__ import annotations + +import argparse +import re +import subprocess +import sys +from pathlib import Path + +from pinpredict_hooks._common import emit + +PROG = "no-production-newtonsoft" +LABEL = "production newtonsoft" + +DEFAULT_TOKEN = "newtonsoft" +DEFAULT_SOURCE_GLOBS = ("*.cs", "*.csproj", "*.props", "*.targets") + + +def central_version_pattern(package: str) -> re.Pattern[str]: + """Match a `` declaration.""" + return re.compile( + rf"<\s*PackageVersion\b[^>]*\bInclude\s*=\s*['\"]{re.escape(package)}['\"][^>]*/?\s*>", + re.IGNORECASE, + ) + + +def is_allowed_path(path: Path, allowed_prefixes: tuple[str, ...]) -> bool: + return path.as_posix().startswith(allowed_prefixes) + + +def is_allowed_central_version( + path: Path, line: str, central_file: str | None, pattern: re.Pattern[str] +) -> bool: + """True for the one line that centrally pins the forbidden package's version. + + Scoped to the named file so a `PackageVersion` element copied into an + ordinary project file is still a violation. + """ + return ( + central_file is not None + and path.as_posix() == central_file + and bool(pattern.search(line)) + ) + + +def tracked_sources(root: Path, globs: tuple[str, ...]) -> list[Path]: + result = subprocess.run( + ["git", "-C", str(root), "ls-files", "--", *globs], + check=True, + capture_output=True, + text=True, + ) + return [Path(line) for line in result.stdout.splitlines() if line] + + +def find_static_violations( + root: Path, + paths: list[Path], + token: str, + allowed_prefixes: tuple[str, ...], + central_file: str | None, + pattern: re.Pattern[str], +) -> list[tuple[Path, int]]: + needle = token.casefold() + failures: list[tuple[Path, int]] = [] + for path in paths: + if is_allowed_path(path, allowed_prefixes): + continue + try: + text = (root / path).read_text(encoding="utf-8") + except (FileNotFoundError, UnicodeDecodeError): + # A tracked-but-absent path (mid-rebase) or a mislabeled binary is + # not this hook's failure to report. + continue + for number, line in enumerate(text.splitlines(), 1): + if needle in line.casefold() and not is_allowed_central_version( + path, line, central_file, pattern + ): + failures.append((path, number)) + return failures + + +def build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser( + prog=PROG, description=__doc__ or "", allow_abbrev=False + ) + parser.add_argument( + "--root", + default=".", + type=Path, + help="repository root to scan (default: the working directory)", + ) + parser.add_argument( + "--token", + default=DEFAULT_TOKEN, + help=f"case-insensitive token that marks a violation (default: {DEFAULT_TOKEN})", + ) + parser.add_argument( + "--package", + default=None, + metavar="ID", + help="package id for the central-version exemption (default: .Json)", + ) + parser.add_argument( + "--allow-prefix", + action="append", + default=[], + metavar="PREFIX", + help="path prefix where a reference is permitted, e.g. Acme.Tests/ (repeatable)", + ) + parser.add_argument( + "--central-version-file", + default=None, + metavar="PATH", + help="file permitted to declare the package's central PackageVersion", + ) + parser.add_argument( + "--source-glob", + action="append", + default=[], + metavar="GLOB", + help=f"git pathspec to scan (repeatable; default: {' '.join(DEFAULT_SOURCE_GLOBS)})", + ) + # pre-commit may pass matched filenames even with pass_filenames: false; + # accept and ignore them so the hook stays robust. + parser.add_argument("filenames", nargs="*", help=argparse.SUPPRESS) + return parser + + +def main(argv: list[str] | None = None) -> int: + args = build_parser().parse_args(argv) + + globs = tuple(args.source_glob) or DEFAULT_SOURCE_GLOBS + package = args.package or f"{args.token}.Json" + pattern = central_version_pattern(package) + allowed_prefixes = tuple(args.allow_prefix) + + sources = tracked_sources(args.root, globs) + failures = find_static_violations( + args.root, + sources, + args.token, + allowed_prefixes, + args.central_version_file, + pattern, + ) + + if failures: + emit( + [ + f"{path}:{number} references {args.token!r} — production code must use " + "System.Text.Json; compatibility references are allowed only under " + f"{', '.join(allowed_prefixes) or 'no approved prefix'}" + for path, number in failures + ], + LABEL, + ) + print( + f"\n{len(failures)} production {args.token} reference(s) in " + f"{len({path for path, _ in failures})} file(s).", + file=sys.stderr, + ) + return 1 + + print(f"{PROG}: no direct production {args.token} references in {len(sources)} file(s).") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/pinpredict_hooks/stevedore_release_scope.py b/pinpredict_hooks/stevedore_release_scope.py index f594eef..b380777 100644 --- a/pinpredict_hooks/stevedore_release_scope.py +++ b/pinpredict_hooks/stevedore_release_scope.py @@ -29,26 +29,17 @@ from __future__ import annotations import argparse -import os import sys from pathlib import Path from typing import Any -import yaml +from pinpredict_hooks._common import MISSING, emit as _emit, load_yaml as _load_yaml -MISSING = object() +PROG = "stevedore-release-scope" def load_yaml(path: Path) -> Any: - """Parse `path`, returning MISSING when the file does not exist.""" - try: - text = path.read_text(encoding="utf-8") - except FileNotFoundError: - return MISSING - try: - return yaml.safe_load(text) - except yaml.YAMLError as error: - raise SystemExit(f"stevedore-release-scope: {path} is not valid YAML: {error}") + return _load_yaml(path, PROG) def image_ids(catalog: Any) -> list[str]: @@ -157,10 +148,7 @@ def check_selectors( def emit(failures: list[str]) -> None: - github = os.environ.get("GITHUB_ACTIONS") == "true" - for failure in failures: - prefix = "::error::" if github else "ERROR: " - print(f"{prefix}stevedore release scope: {failure}") + _emit(failures, "stevedore release scope") def build_parser() -> argparse.ArgumentParser: diff --git a/pyproject.toml b/pyproject.toml index 8387f59..a5ffcec 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -17,6 +17,7 @@ dependencies = ["PyYAML>=6"] [project.scripts] stevedore-release-scope = "pinpredict_hooks.stevedore_release_scope:main" service-yaml-check = "pinpredict_hooks.service_yaml_check:main" +no-production-newtonsoft = "pinpredict_hooks.no_production_newtonsoft:main" [tool.setuptools] packages = ["pinpredict_hooks"] diff --git a/tests/test_no_production_newtonsoft.py b/tests/test_no_production_newtonsoft.py new file mode 100644 index 0000000..a7c651a --- /dev/null +++ b/tests/test_no_production_newtonsoft.py @@ -0,0 +1,184 @@ +#!/usr/bin/env python3 +"""Regression tests for the no-production-newtonsoft hook. + +The static-scan cases here were ported from trading's +`scripts/ci/test_check_no_production_newtonsoft.py` when the scan moved into +this repo. Trading kept the package-graph tests, which cover the `dotnet`- +dependent half that stays in its CI. + +These need a real git repo because the hook enumerates files with +`git ls-files` — an intentional part of its contract (whole-tree scanning, so a +violation in an untouched file still fails). `git init` on a temp dir is a few +milliseconds, so the suite still runs as a hook in this repo. +""" + +from __future__ import annotations + +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from pinpredict_hooks.no_production_newtonsoft import ( # noqa: E402 + central_version_pattern, + find_static_violations, + main, +) + +ALLOWED = ("Acme.Tests/", "Acme.Benchmarks/") +CENTRAL = "Directory.Packages.props" +PATTERN = central_version_pattern("Newtonsoft.Json") + + +class Repo: + """A temp git repo holding the tracked files the hook scans.""" + + def __init__(self, root: Path) -> None: + self.root = root + + def write(self, relative: str, content: str) -> None: + target = self.root / relative + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + + def track(self) -> None: + subprocess.run( + ["git", "-c", "init.defaultBranch=main", "init", "-q", "--template=", "."], + cwd=self.root, + check=True, + ) + subprocess.run(["git", "add", "-A"], cwd=self.root, check=True) + + def run(self, *args: str) -> int: + return main( + [ + f"--root={self.root}", + *(f"--allow-prefix={prefix}" for prefix in ALLOWED), + f"--central-version-file={CENTRAL}", + *args, + ] + ) + + +class StaticScanTests(unittest.TestCase): + def make_repo(self) -> Repo: + directory = tempfile.TemporaryDirectory() + self.addCleanup(directory.cleanup) + return Repo(Path(directory.name)) + + def scan(self, repo: Repo, paths: list[str]) -> list[tuple[Path, int]]: + return find_static_violations( + repo.root, [Path(p) for p in paths], "newtonsoft", ALLOWED, CENTRAL, PATTERN + ) + + def test_scan_is_case_insensitive_and_includes_build_files(self) -> None: + repo = self.make_repo() + repo.write("Acme.Api/Program.cs", "using NEWTONSOFT.Json;\n") + repo.write( + "Acme.Api/Acme.Api.csproj", + '' + "\n", + ) + repo.write("Directory.Build.props", '\n') + self.assertEqual( + [ + (Path("Acme.Api/Program.cs"), 1), + (Path("Acme.Api/Acme.Api.csproj"), 1), + (Path("Directory.Build.props"), 1), + ], + self.scan( + repo, + [ + "Acme.Api/Program.cs", + "Acme.Api/Acme.Api.csproj", + "Directory.Build.props", + ], + ), + ) + + def test_scan_keeps_only_explicit_compatibility_exceptions(self) -> None: + repo = self.make_repo() + repo.write("Acme.Tests/JsonTests.cs", "using Newtonsoft.Json;\n") + repo.write("Acme.Benchmarks/Bench.cs", "using Newtonsoft.Json;\n") + repo.write( + CENTRAL, + '\n\n\n', + ) + self.assertEqual( + [], + self.scan( + repo, + ["Acme.Tests/JsonTests.cs", "Acme.Benchmarks/Bench.cs", CENTRAL], + ), + ) + + def test_a_lookalike_prefix_is_not_exempt(self) -> None: + """`Acme.TestsSupport/` must not inherit `Acme.Tests/`'s exemption.""" + repo = self.make_repo() + repo.write("Acme.TestSupport/Helper.cs", "using Newtonsoft.Json;\n") + self.assertEqual( + [(Path("Acme.TestSupport/Helper.cs"), 1)], + self.scan(repo, ["Acme.TestSupport/Helper.cs"]), + ) + + def test_central_version_exemption_is_scoped_to_the_named_file(self) -> None: + repo = self.make_repo() + line = '\n' + repo.write("Acme.Api/Acme.Api.csproj", line) + self.assertEqual( + [(Path("Acme.Api/Acme.Api.csproj"), 1)], + self.scan(repo, ["Acme.Api/Acme.Api.csproj"]), + ) + + def test_central_file_still_fails_on_a_non_packageversion_reference(self) -> None: + repo = self.make_repo() + repo.write(CENTRAL, '\n') + self.assertEqual([(Path(CENTRAL), 1)], self.scan(repo, [CENTRAL])) + + def test_end_to_end_clean_tree_passes(self) -> None: + repo = self.make_repo() + repo.write("Acme.Api/Program.cs", "using System.Text.Json;\n") + repo.write("Acme.Tests/JsonTests.cs", "using Newtonsoft.Json;\n") + repo.track() + self.assertEqual(0, repo.run()) + + def test_end_to_end_violation_fails(self) -> None: + repo = self.make_repo() + repo.write("Acme.Api/Program.cs", "using Newtonsoft.Json;\n") + repo.track() + self.assertEqual(1, repo.run()) + + def test_an_untouched_file_still_fails_the_whole_tree_scan(self) -> None: + """The policy is a tree property — staged-file scoping would hide this.""" + repo = self.make_repo() + repo.write("Acme.Api/Legacy.cs", "using Newtonsoft.Json;\n") + repo.write("Acme.Api/Program.cs", "using System.Text.Json;\n") + repo.track() + self.assertEqual(1, repo.run("Acme.Api/Program.cs")) + + def test_a_repo_with_no_matching_sources_is_a_clean_no_op(self) -> None: + repo = self.make_repo() + repo.write("README.md", "# nothing to scan\n") + repo.track() + self.assertEqual(0, repo.run()) + + def test_no_allow_prefix_permits_nothing(self) -> None: + repo = self.make_repo() + repo.write("Acme.Tests/JsonTests.cs", "using Newtonsoft.Json;\n") + repo.track() + self.assertEqual(1, main([f"--root={repo.root}"])) + + def test_token_is_configurable(self) -> None: + repo = self.make_repo() + repo.write("Acme.Api/Program.cs", "using ServiceStack.Text;\n") + repo.track() + self.assertEqual(1, main([f"--root={repo.root}", "--token=servicestack"])) + self.assertEqual(0, main([f"--root={repo.root}"])) + + +if __name__ == "__main__": + unittest.main()