From 1439c0f5c0f9892629e92e2a1aad682aed7277af Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Fri, 28 Aug 2026 13:58:44 +0200 Subject: [PATCH] Make the allowlist inheritable, and cache the quarantine lookups Two things that were fine in utils and would not have been in the next forty repos. **The allowlist is now fleet-wide plus per-repo.** `typescript-eslint` blocks TS 7 in EVERY repo that pins typescript -- eight of them and counting -- and a repo-only allowlist meant hand-copying one entry into eight files that then rot independently. That is the four-way fork of the 250-line cap, reproduced inside the mechanism built to avoid it. `~/utils/dependency-freshness.allowlist.yaml` is inherited by every repo; a repo entry for the same `ecosystem:package` overrides it. Inherited entries are exempt from the no-longer-needed check everywhere except the repo that owns the file. Without that carve-out a fleet-wide hold on typescript exits 2 in every Flutter app -- it excuses nothing there, which is the normal case, not rot -- and those repos become uncommittable. Confirmed against punchme, which now exits 0 while utils still reports the entry as in use. **`quarantine.py` bypassed the cache entirely.** It ran for every npm finding and fetched the FULL packument -- 7MB for react, 16MB for typescript -- with no TTL and no reuse, inside a pre-commit hook. One finding in utils, but the JS repos ahead have 16 to 37 each. Now cached under the normal 6h TTL, keyed `@`: the tag has to be part of the key rather than clamped afterwards, because clamping returns the ceiling and the ceiling is frequently the very release the quarantine is excluding. **Both cache and shared allowlist are now redirected by AUTOUSE fixtures.** A quarantine test had already seeded a fake answer for package "x" into the real ~/.cache/dep-freshness/registry.json, and the next run read its own pollution back and passed for the wrong reason. Removed the entry; the fixture makes it unrepeatable. 292 tests, 100% branch coverage on every module. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CRykabMwPgDzFHcARoqpaK --- dep_freshness/_tables.py | 7 ++++ dep_freshness/allowlist.py | 37 +++++++++++++++++-- dep_freshness/check.py | 22 +++++++++--- dep_freshness/quarantine.py | 31 ++++++++++++++-- dep_freshness/tests/conftest.py | 30 ++++++++++++++-- dep_freshness/tests/test_allowlist.py | 43 ++++++++++++++++++++++ dep_freshness/tests/test_check.py | 40 +++++++++++++++++++++ dep_freshness/tests/test_quarantine.py | 49 ++++++++++++++++++++++++++ dependency-freshness.allowlist.yaml | 14 ++++++-- 9 files changed, 259 insertions(+), 14 deletions(-) diff --git a/dep_freshness/_tables.py b/dep_freshness/_tables.py index 803fb41..1bd1cf3 100644 --- a/dep_freshness/_tables.py +++ b/dep_freshness/_tables.py @@ -114,5 +114,12 @@ PUB_SDK_PACKAGES: Final = frozenset({ PUB_OVERRIDE_KEY: Final = "dependency_overrides" ALLOWLIST_FILE: Final = "dependency-freshness.allowlist.yaml" + +# A blocker like "typescript-eslint does not support TS 7" holds in EVERY repo +# that pins typescript, so a repo-only allowlist would mean hand-copying one +# entry into eight files that then all rot independently -- the four-way fork +# of the 250-line cap, in the mechanism built to avoid it. The shared file in +# ~/utils is inherited; a repo entry for the same package overrides it. +SHARED_ALLOWLIST_ENV: Final = "DEP_FRESHNESS_SHARED_ALLOWLIST" ALLOWLIST_MAX_DAYS: Final = 90 TRANSITIVE_PREFIX: Final = "transitive:" diff --git a/dep_freshness/allowlist.py b/dep_freshness/allowlist.py index 85ee9c9..5aca87f 100644 --- a/dep_freshness/allowlist.py +++ b/dep_freshness/allowlist.py @@ -20,7 +20,13 @@ from pathlib import Path import yaml -from dep_freshness._tables import ALLOWLIST_FILE, ALLOWLIST_MAX_DAYS +import os + +from dep_freshness._tables import ( + ALLOWLIST_FILE, + ALLOWLIST_MAX_DAYS, + SHARED_ALLOWLIST_ENV, +) from dep_freshness.models import Exception_ REQUIRED = ("ecosystem", "package", "pinned", "reason", "blocked_by") @@ -34,6 +40,18 @@ def path_for(root: Path) -> Path: return root / ALLOWLIST_FILE +def shared_path() -> Path: + """The fleet-wide allowlist that every repo inherits. + + Lives beside the gate it belongs to. Overridable so a test -- or a machine + that keeps utils somewhere else -- is not forced to touch the real one. + """ + override = os.environ.get(SHARED_ALLOWLIST_ENV) + if override: + return Path(override).expanduser() + return Path(__file__).resolve().parent.parent / ALLOWLIST_FILE + + def _parse_date(raw: object, where: str) -> date: if isinstance(raw, date) and not isinstance(raw, datetime): return raw @@ -96,8 +114,21 @@ def check_expiry(entry: Exception_, today: date | None = None) -> None: def load(root: Path) -> list[Exception_]: - """Every exception declared for `root`; [] when there is no allowlist.""" - source = path_for(root) + """Every exception in force for `root`: the shared set, then the repo's. + + A repo entry for the same `ecosystem:package` replaces the shared one, so a + repo can always be stricter or state its own reason -- but nobody has to + copy a fleet-wide blocker into forty files to be able to commit. + """ + entries = { + f"{e.ecosystem}:{e.package}": e for e in _load_file(shared_path()) + } + for entry in _load_file(path_for(root)): + entries[f"{entry.ecosystem}:{entry.package}"] = entry + return list(entries.values()) + + +def _load_file(source: Path) -> list[Exception_]: if not source.is_file(): return [] try: diff --git a/dep_freshness/check.py b/dep_freshness/check.py index 126d68c..26def52 100644 --- a/dep_freshness/check.py +++ b/dep_freshness/check.py @@ -21,7 +21,7 @@ import sys from dep_freshness import report from dep_freshness._tables import NPM -from dep_freshness.allowlist import AllowlistError, load +from dep_freshness.allowlist import AllowlistError, load, shared_path from dep_freshness.discover import find_manifests, is_manifest, parse_manifest from dep_freshness.evaluate import judge from dep_freshness.models import Finding, Severity @@ -43,12 +43,19 @@ def repo_root(start: Path) -> Path: return Path(result.stdout.strip() or start) -def _excuse(findings: list[Finding], entries) -> tuple[list[Finding], list]: +def _excuse( + findings: list[Finding], entries, shared: Path | None = None +) -> tuple[list[Finding], list]: """Split findings into (still failing, excused), and flag dead entries. A `transitive:` entry clears itself the moment its dependency is no longer stale — that is what makes it a predicate rather than a date. An entry with nothing left to excuse is allowlist rot and exits 2. + + Inherited entries are exempt from that rot check everywhere except the + repo that owns the shared file. A fleet-wide hold on `typescript` + legitimately excuses nothing in a Flutter app, and failing there would + make every repo without that dependency uncommittable. """ by_label = {f"{e.ecosystem}:{e.package}": e for e in entries} used: set[str] = set() @@ -64,7 +71,11 @@ def _excuse(findings: list[Finding], entries) -> tuple[list[Finding], list]: finding.dep, finding.severity, finding.latest, finding.detail, excused=entry.reason, )) - dead = [e for e in entries if f"{e.ecosystem}:{e.package}" not in used] + dead = [ + e for e in entries + if f"{e.ecosystem}:{e.package}" not in used + and (shared is None or e.source != shared) + ] return failing, dead @@ -149,7 +160,10 @@ def main(argv: list[str] | None = None) -> int: resolver = Resolver(refresh=args.refresh) findings = collect(_targets(args, root), resolver) - failing, dead = _excuse(findings, entries) + shared = shared_path() + failing, dead = _excuse( + findings, entries, shared=None if root == shared.parent else shared + ) still_blocking = {f.label: True for f in findings} report.exceptions_block(entries, still_blocking) diff --git a/dep_freshness/quarantine.py b/dep_freshness/quarantine.py index 3a295d3..010e303 100644 --- a/dep_freshness/quarantine.py +++ b/dep_freshness/quarantine.py @@ -17,9 +17,14 @@ from __future__ import annotations from datetime import datetime, timedelta, timezone from dep_freshness._tables import NPM_API, NPM_QUARANTINE_HOURS +from dep_freshness.cache import Cache from dep_freshness.registries.http import Offline, get_json from dep_freshness.versions import newest_stable, parse +# Its own cache namespace: the answer here is narrower than `npm/` and +# the two must not overwrite each other. +CACHE_ECOSYSTEM = "npm-installable" + def cutoff(now: datetime | None = None) -> datetime: """Publishes at or after this instant are still quarantined.""" @@ -35,7 +40,10 @@ def _parse(stamp: str) -> datetime | None: def installable_latest( - name: str, ceiling: str, now: datetime | None = None + name: str, + ceiling: str, + now: datetime | None = None, + cache: Cache | None = None, ) -> str | None: """Newest stable release published before the quarantine cutoff. @@ -50,11 +58,25 @@ def installable_latest( registry has no `time` map), which callers must treat as "no change" -- narrowing on a guess would hide a genuinely stale dependency. """ + # Cached under the normal TTL. Without this every gate run in a JS-heavy + # repo re-downloads tens of full packuments, inside a pre-commit hook -- + # the exact cost the 6h cache exists to prevent. + # + # The ceiling is part of the KEY, not applied afterwards: clamping a cached + # answer to the ceiling would return the ceiling itself, and the ceiling is + # frequently the very release the quarantine is excluding. `dist-tags.latest` + # is stable within a TTL, so hits still land across manifests. + store = cache if cache is not None else Cache() + key = f"{name}@{ceiling}" + entry = store.get(CACHE_ECOSYSTEM, key) + if entry is not None and entry.fresh(CACHE_ECOSYSTEM): + return entry.version + url = NPM_API.format(name=name.replace("/", "%2f")) try: payload = get_json(url) except Offline: - return None + return entry.version if entry is not None else None # stale beats none if not payload: return None times = payload.get("time") @@ -73,4 +95,7 @@ def installable_latest( if top is not None and (here is None or here > top): continue old_enough.append(version) - return newest_stable(old_enough) + answer = newest_stable(old_enough) + store.put(CACHE_ECOSYSTEM, key, answer) + store.save() + return answer diff --git a/dep_freshness/tests/conftest.py b/dep_freshness/tests/conftest.py index 2d71651..b4536fb 100644 --- a/dep_freshness/tests/conftest.py +++ b/dep_freshness/tests/conftest.py @@ -46,9 +46,35 @@ def no_network(monkeypatch): http.force_offline(False) -@pytest.fixture +@pytest.fixture(autouse=True) +def no_shared_allowlist(tmp_path, monkeypatch) -> Path: + """Point the fleet-wide allowlist at a path that does not exist. + + AUTOUSE for the same reason as `cache_dir`: repos inherit the real + ~/utils/dependency-freshness.allowlist.yaml, so without this every test + silently reads whatever is currently excused fleet-wide and starts + depending on it. + """ + # In its OWN directory, not tmp_path: the `repo` fixture is tmp_path, and + # the gate exempts inherited entries from the rot check everywhere except + # the repo that owns the shared file. Putting both in one directory makes + # every test look like it is running inside utils. + target = tmp_path / "shared" / "shared-allowlist.yaml" + target.parent.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv("DEP_FRESHNESS_SHARED_ALLOWLIST", str(target)) + return target + + +@pytest.fixture(autouse=True) def cache_dir(tmp_path, monkeypatch) -> Path: - """Point the on-disk registry snapshot at a scratch directory.""" + """Point the on-disk registry snapshot at a scratch directory. + + AUTOUSE, and not negotiable: any code path that constructs a bare `Cache()` + writes to the real ~/.cache/dep-freshness/registry.json otherwise. That + already happened once -- a quarantine test seeded a fake answer for package + "x" into the live cache, and the next run of the same test read its own + pollution back and passed for the wrong reason. + """ target = tmp_path / "cache" monkeypatch.setenv("DEP_FRESHNESS_CACHE", str(target)) return target diff --git a/dep_freshness/tests/test_allowlist.py b/dep_freshness/tests/test_allowlist.py index a328767..cacd05b 100644 --- a/dep_freshness/tests/test_allowlist.py +++ b/dep_freshness/tests/test_allowlist.py @@ -152,3 +152,46 @@ def test_unreadable_yaml_is_an_error(tmp_path): _allowlist(tmp_path, "exceptions: [\n") with pytest.raises(AllowlistError, match="unreadable"): load(tmp_path) + + +def test_the_shared_allowlist_is_inherited(tmp_path, no_shared_allowlist): + """A fleet-wide blocker must not have to be copied into forty repos.""" + no_shared_allowlist.write_text(TRANSITIVE, encoding="utf-8") + entries = load(tmp_path) + assert [(e.ecosystem, e.package) for e in entries] == [ + ("pub", "plugin_platform_interface") + ] + + +def test_a_repo_entry_overrides_the_shared_one(tmp_path, no_shared_allowlist): + no_shared_allowlist.write_text(TRANSITIVE, encoding="utf-8") + _allowlist(tmp_path, TRANSITIVE.replace( + 'reason: "firebase_core 4.2.0 constrains this below latest"', + 'reason: "this repo has its own reason"')) + entries = load(tmp_path) + assert len(entries) == 1 + assert entries[0].reason == "this repo has its own reason" + + +def test_shared_and_repo_entries_for_different_packages_both_apply( + tmp_path, no_shared_allowlist +): + no_shared_allowlist.write_text(TRANSITIVE, encoding="utf-8") + _allowlist(tmp_path, _discretionary(30)) + assert {e.package for e in load(tmp_path)} == { + "plugin_platform_interface", "abandoned" + } + + +def test_a_malformed_shared_allowlist_is_an_error(tmp_path, no_shared_allowlist): + no_shared_allowlist.write_text("exceptions:\n - just-a-string\n", + encoding="utf-8") + with pytest.raises(AllowlistError, match="must be a mapping"): + load(tmp_path) + + +def test_the_shared_path_defaults_to_the_gate_repo(monkeypatch): + from dep_freshness.allowlist import shared_path + monkeypatch.delenv("DEP_FRESHNESS_SHARED_ALLOWLIST", raising=False) + assert shared_path().name == NAME + assert shared_path().parent.name == "utils" diff --git a/dep_freshness/tests/test_check.py b/dep_freshness/tests/test_check.py index a9f5d63..1554b8c 100644 --- a/dep_freshness/tests/test_check.py +++ b/dep_freshness/tests/test_check.py @@ -186,3 +186,43 @@ def _offline(_ecosystem, _name): def test_the_cache_directory_is_honoured(cache_dir): assert os.environ["DEP_FRESHNESS_CACHE"] == str(cache_dir) + + +SHARED = """\ +exceptions: + - ecosystem: pub + package: some_other_package + pinned: "1.0.0" + reason: "a fleet-wide hold on something this repo does not use" + blocked_by: "transitive:whatever@1.0.0" +""" + + +def test_an_inherited_entry_this_repo_does_not_need_is_not_rot( + repo, run, no_shared_allowlist +): + """A fleet-wide hold on typescript excuses nothing in a Flutter app. + + Treating that as a dead entry would make every repo without the dependency + uncommittable -- the shared allowlist would be unusable for the exact case + it exists for. + """ + no_shared_allowlist.write_text(SHARED, encoding="utf-8") + write(repo, "pubspec.yaml", CURRENT_PUBSPEC) + assert run("--all") == 0 + + +def test_a_repo_local_entry_with_nothing_to_excuse_is_still_rot( + repo, run, no_shared_allowlist +): + no_shared_allowlist.write_text(SHARED, encoding="utf-8") + write(repo, "pubspec.yaml", CURRENT_PUBSPEC) + write(repo, ALLOWLIST, """\ +exceptions: + - ecosystem: pub + package: http + pinned: "1.5.0" + reason: "this repo's own stale entry" + blocked_by: "transitive:x@1.0.0" +""") + assert run("--all") == 2 diff --git a/dep_freshness/tests/test_quarantine.py b/dep_freshness/tests/test_quarantine.py index a251db5..190c917 100644 --- a/dep_freshness/tests/test_quarantine.py +++ b/dep_freshness/tests/test_quarantine.py @@ -166,3 +166,52 @@ def test_a_repo_pinned_to_the_newest_installable_version_passes( '{"dependencies": {"@testing-library/react": "16.3.2"}}') monkeypatch.chdir(repo) assert check.main(["--all"]) == 0 + + +def test_the_answer_is_cached_so_a_second_run_makes_no_request(packument, + cache_dir): + """A 7MB packument per npm finding, per run, inside a pre-commit hook is + exactly what the 6h cache exists to prevent.""" + from dep_freshness.cache import Cache + + store = Cache(cache_dir / "registry.json") + packument(PACKUMENT) + assert quarantine.installable_latest("x", "16.3.3", now=NOW, + cache=store) == "16.3.2" + + packument(None) # any further fetch would now answer "no such package" + assert quarantine.installable_latest("x", "16.3.3", now=NOW, + cache=store) == "16.3.2" + + +def test_a_different_dist_tag_is_a_different_cache_entry(packument, cache_dir): + """The ceiling is part of the key: clamping a cached answer to it would + return the ceiling, which is often the release being excluded.""" + from dep_freshness.cache import Cache + + store = Cache(cache_dir / "registry.json") + packument(PACKUMENT) + assert quarantine.installable_latest("x", "16.3.2", now=NOW, + cache=store) == "16.3.2" + assert store.get(quarantine.CACHE_ECOSYSTEM, "x@16.3.3") is None + + +def test_a_stale_cache_entry_is_served_when_the_registry_is_down(packument, + cache_dir): + from dep_freshness.cache import Cache + from dep_freshness.registries.http import Offline + + store = Cache(cache_dir / "registry.json") + store.put(quarantine.CACHE_ECOSYSTEM, "x@16.3.3", "16.3.2") + store._data[f"{quarantine.CACHE_ECOSYSTEM}/x@16.3.3"]["fetched_at"] = 0.0 + + def down(*_a, **_k): + raise Offline("no network") + + monkey = down + import dep_freshness.quarantine as q + original, q.get_json = q.get_json, monkey + try: + assert q.installable_latest("x", "16.3.3", cache=store) == "16.3.2" + finally: + q.get_json = original diff --git a/dependency-freshness.allowlist.yaml b/dependency-freshness.allowlist.yaml index 8d99094..92edcfc 100644 --- a/dependency-freshness.allowlist.yaml +++ b/dependency-freshness.allowlist.yaml @@ -1,5 +1,11 @@ # Dependencies deliberately held below latest stable, and why. # +# THIS IS THE FLEET-WIDE FILE. Every repo inherits it; a repo's own +# dependency-freshness.allowlist.yaml adds to it, and an entry there for the +# same ecosystem:package overrides the one here. A blocker like "typescript-eslint +# does not support TS 7" holds in every repo that pins typescript, and copying +# it into eight of them would give eight entries that rot independently. +# # Two entry classes, and the distinction is load-bearing: # # blocked_by: transitive:@ @@ -24,6 +30,10 @@ exceptions: typescript-eslint 8.68.0 -- itself the latest release -- declares `typescript: ">=4.8.4 <6.1.0"` and throws "typescript-eslint does not support TS 7.0" when eslint loads its config. No 9.x exists. Taking TS 7 - would mean dropping every type-aware lint rule from ts_core and web_ui - until upstream catches up, so the compiler waits for the linter. + would mean dropping every type-aware lint rule from every repo that pins + it, so the compiler waits for the linter. + Held fleet-wide: this blocks ts_core, web_ui, konbini-67, iron-and-anvil, + awesome-mcp-explorer, europe-county-map and every other repo pinning + typescript, which is exactly why the entry lives here and not in eight + separate files. blocked_by: "transitive:typescript-eslint@8.68.0"