From 6b830ec70c1e98737fb67e90c6f221ac95a9c7ee Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 17:45:47 -0600 Subject: [PATCH 01/19] feat(py): semantic_layer over inline measures --- pkg-py/src/commons/_measures.py | 82 +++++++++++++++++++++++++++++++++ pkg-py/tests/test_measures.py | 76 ++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+) diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index d75b912b..76da87c7 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -14,6 +14,7 @@ import inspect from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass +from types import MappingProxyType from typing import ( Annotated, Any, @@ -349,3 +350,84 @@ def as_measure(obj: Any) -> Measure | None: def _humanize(name: str) -> str: return name.replace("_", " ") + + +@dataclass(frozen=True) +class SemanticLayer: + """The trusted calculations an agent can run. + + ``source_text`` holds the source of the measures and the module-level + helpers they call, keyed by Python name. Only text is kept: the agent's + worker session reads measure definitions but never receives a callable. + """ + + measures: Mapping[str, Measure] + source_text: Mapping[str, str] + + def __len__(self) -> int: + return len(self.measures) + + def __repr__(self) -> str: + count = len(self.measures) + plural = "" if count == 1 else "s" + return f"A commons semantic layer with {count} measure{plural}." + + +def semantic_layer(*items: Any) -> SemanticLayer: + """Collect measures into a semantic layer. + + Each item is a measure, a list of measures, a module, or a path to a + Python file or a directory of them. Directory searches are not recursive. + """ + measures: dict[str, Measure] = {} + source_text: dict[str, str] = {} + duplicates: list[str] = [] + + for item in items: + found, sources = _collect(item) + for record in found: + if record.name in measures: + duplicates.append(record.name) + measures[record.name] = record + for name, text in sources.items(): + # First definition wins, matching R's de-duplication of harvested + # sources across files. + source_text.setdefault(name, text) + + if duplicates: + raise ValueError( + f"Measure names must be unique; duplicated: " + f"{', '.join(sorted(set(duplicates)))}." + ) + + return SemanticLayer( + measures=MappingProxyType(measures), + source_text=MappingProxyType(source_text), + ) + + +def _collect(item: Any) -> tuple[list[Measure], dict[str, str]]: + if isinstance(item, (list, tuple)): + measures: list[Measure] = [] + sources: dict[str, str] = {} + for entry in item: + found, text = _collect(entry) + measures.extend(found) + sources.update(text) + return measures, sources + + record = as_measure(item) + if record is None: + raise TypeError( + f"Every item in semantic_layer() must be a measure, a list of " + f"measures, a module, or a path; got {item!r}.\n" + f"Decorate the function with @measure to make it one." + ) + return [record], {record.func.__name__: _source_text(record.func)} + + +def _source_text(func: Callable[..., Any]) -> str: + try: + return inspect.getsource(func) + except (OSError, TypeError): + return f"# source unavailable for {func.__name__}" diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index 85a18ef1..c8ec1ea7 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -16,6 +16,7 @@ as_measure, measure, measure_schema_text, + semantic_layer, ) from ._shared import load_shared_fixture @@ -547,3 +548,78 @@ def regional_orders( rendered = measure_schema_text(_as_measure(regional_orders)) assert "regions (array of {EMEA, AMER}, optional) Enum array, nullable." in rendered +def test_semantic_layer_keys_measures_by_name() -> None: + @measure(description="Count of orders.") + def order_count() -> int: + return 1 + + layer = semantic_layer(order_count) + + assert list(layer.measures) == ["order_count"] + assert layer.measures["order_count"].description == "Count of orders." + + +def test_semantic_layer_accepts_a_list_of_measures() -> None: + @measure(description="Count of orders.") + def order_count() -> int: + return 1 + + @measure(description="Total revenue.") + def total_revenue() -> int: + return 2 + + layer = semantic_layer([order_count, total_revenue]) + + assert list(layer.measures) == ["order_count", "total_revenue"] + + +def test_semantic_layer_accepts_a_bare_measure_record() -> None: + layer = semantic_layer(_count_measure()) + + assert list(layer.measures) == ["order_count"] + + +def test_semantic_layer_is_empty_with_no_arguments() -> None: + layer = semantic_layer() + + assert len(layer) == 0 + assert layer.measures == {} + + +def test_semantic_layer_rejects_a_non_measure() -> None: + with pytest.raises(TypeError, match="2026"): + semantic_layer(2026) + + +def test_semantic_layer_rejects_an_undecorated_function() -> None: + def helper() -> int: + return 1 + + with pytest.raises(TypeError, match="helper"): + semantic_layer(helper) + + +def test_semantic_layer_rejects_duplicate_names() -> None: + @measure(description="Count of orders.") + def order_count() -> int: + return 1 + + with pytest.raises(ValueError, match="order_count"): + semantic_layer(order_count, order_count) + + +def test_semantic_layer_harvests_inline_measure_source() -> None: + @measure(description="Count of orders.") + def order_count() -> int: + return 1 + + layer = semantic_layer(order_count) + + assert "def order_count()" in layer.source_text["order_count"] + + +def test_semantic_layer_reports_its_size() -> None: + layer = semantic_layer(_count_measure()) + + assert len(layer) == 1 + assert "1 measure" in repr(layer) From ab279bb5d7de3cf4f32df1f24d9af0859520365d Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 17:50:01 -0600 Subject: [PATCH 02/19] fix: add suggestion line to duplicate measure names error --- pkg-py/src/commons/_measures.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 76da87c7..d7698eb9 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -397,7 +397,9 @@ def semantic_layer(*items: Any) -> SemanticLayer: if duplicates: raise ValueError( f"Measure names must be unique; duplicated: " - f"{', '.join(sorted(set(duplicates)))}." + f"{', '.join(sorted(set(duplicates)))}.\n" + f"Give one of the colliding measures a distinct name with " + f"@measure(name=...)." ) return SemanticLayer( From 3c5b6830362ec329ba6fc0ebab9a757aba34cad8 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 18:00:09 -0600 Subject: [PATCH 03/19] fix(py): nested list collection matches top-level first-definition-wins _collect()'s list/tuple branch merged harvested source with sources.update(), so within a nested list the last definition of a Python name won; semantic_layer() itself uses setdefault, so the first wins. Use the same rule in both places so source_text is independent of how measures are nested. --- pkg-py/src/commons/_measures.py | 4 +++- pkg-py/tests/test_measures.py | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index d7698eb9..ccf9e1c4 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -415,7 +415,9 @@ def _collect(item: Any) -> tuple[list[Measure], dict[str, str]]: for entry in item: found, text = _collect(entry) measures.extend(found) - sources.update(text) + for name, name_text in text.items(): + # First definition wins, matching semantic_layer()'s rule. + sources.setdefault(name, name_text) return measures, sources record = as_measure(item) diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index c8ec1ea7..8f03e123 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -618,6 +618,33 @@ def order_count() -> int: assert "def order_count()" in layer.source_text["order_count"] +def test_collect_nested_list_keeps_first_definition_wins() -> None: + # Both functions are named `calc`, so they collide in `source_text` + # (keyed by Python name) without colliding in `measures` (keyed by the + # distinct `name=` given to each). + def make_first() -> Any: + @measure(description="First.", name="first") + def calc() -> int: + return 1 + + return as_measure(calc) + + def make_second() -> Any: + @measure(description="Second.", name="second") + def calc() -> int: + return 2 + + return as_measure(calc) + + first, second = make_first(), make_second() + + top_level = semantic_layer(first, second) + nested = semantic_layer([first, second]) + + assert nested.source_text["calc"] == top_level.source_text["calc"] + assert "return 1" in nested.source_text["calc"] + + def test_semantic_layer_reports_its_size() -> None: layer = semantic_layer(_count_measure()) From 3e7b94947c7a419ba85503e3c21acabefb405cf4 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 18:14:03 -0600 Subject: [PATCH 04/19] style(py): restore blank-line spacing lost in the rebase's conflict resolution --- pkg-py/tests/test_measures.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index 8f03e123..c6c03afe 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -548,6 +548,8 @@ def regional_orders( rendered = measure_schema_text(_as_measure(regional_orders)) assert "regions (array of {EMEA, AMER}, optional) Enum array, nullable." in rendered + + def test_semantic_layer_keys_measures_by_name() -> None: @measure(description="Count of orders.") def order_count() -> int: From 169c80cae4ba8a4767098ba979be4ead3ca60e68 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 21:20:47 -0600 Subject: [PATCH 05/19] feat(py): read measures from modules, files, and directories --- pkg-py/src/commons/_measures.py | 80 ++++++++++++++++++- pkg-py/tests/measure_sources/nested/orders.py | 12 +++ pkg-py/tests/measure_sources/orders.py | 20 +++++ pkg-py/tests/measure_sources/revenue.py | 8 ++ pkg-py/tests/test_measures.py | 80 +++++++++++++++++++ 5 files changed, 199 insertions(+), 1 deletion(-) create mode 100644 pkg-py/tests/measure_sources/nested/orders.py create mode 100644 pkg-py/tests/measure_sources/orders.py create mode 100644 pkg-py/tests/measure_sources/revenue.py diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index ccf9e1c4..fd4eafe9 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -11,10 +11,15 @@ from __future__ import annotations +import hashlib +import importlib.util import inspect +import os +import sys from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass -from types import MappingProxyType +from pathlib import Path +from types import MappingProxyType, ModuleType from typing import ( Annotated, Any, @@ -378,6 +383,9 @@ def semantic_layer(*items: Any) -> SemanticLayer: Each item is a measure, a list of measures, a module, or a path to a Python file or a directory of them. Directory searches are not recursive. + + A measure that calls a helper defined in another file imports it, the way + any Python module does. """ measures: dict[str, Measure] = {} source_text: dict[str, str] = {} @@ -420,6 +428,12 @@ def _collect(item: Any) -> tuple[list[Measure], dict[str, str]]: sources.setdefault(name, name_text) return measures, sources + if isinstance(item, ModuleType): + return _from_module(item) + + if isinstance(item, (str, os.PathLike)): + return _from_path(Path(item)) + record = as_measure(item) if record is None: raise TypeError( @@ -430,6 +444,70 @@ def _collect(item: Any) -> tuple[list[Measure], dict[str, str]]: return [record], {record.func.__name__: _source_text(record.func)} +def _from_path(path: Path) -> tuple[list[Measure], dict[str, str]]: + if not path.exists(): + raise ValueError( + f"Path does not exist: {path}.\n" + f"semantic_layer() takes measures, modules, Python files, or " + f"directories of them." + ) + + # Not recursive, and __init__.py is skipped: a directory of measure files + # is a directory, not a package. + files = ( + sorted( + entry + for entry in path.iterdir() + if entry.suffix == ".py" and entry.name != "__init__.py" + ) + if path.is_dir() + else [path] + ) + + measures: list[Measure] = [] + sources: dict[str, str] = {} + for file in files: + found, text = _from_module(_load_module_from_path(file)) + measures.extend(found) + sources.update(text) + return measures, sources + + +def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: + """Harvest a module's measures and the source of every function it defines. + + Helpers are harvested too, so the worker session can show the reasoning a + measure delegates to. Imported names are skipped: they belong to the + module they were defined in. + """ + measures: list[Measure] = [] + sources: dict[str, str] = {} + for name, value in vars(module).items(): + if not inspect.isfunction(value) or value.__module__ != module.__name__: + continue + sources[name] = _source_text(value) + record = as_measure(value) + if record is not None: + measures.append(record) + return measures, sources + + +def _load_module_from_path(path: Path) -> ModuleType: + # The digest keeps two files with the same stem from overwriting each + # other in sys.modules; registering before exec_module() is what lets + # dataclasses and typing resolve names back to the module while it is + # still executing. + digest = hashlib.sha256(str(path.resolve()).encode()).hexdigest()[:8] + name = f"commons._measure_sources.{path.stem}_{digest}" + spec = importlib.util.spec_from_file_location(name, path) + if spec is None or spec.loader is None: + raise ValueError(f"Cannot read measures from {path}: not a Python file.") + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + def _source_text(func: Callable[..., Any]) -> str: try: return inspect.getsource(func) diff --git a/pkg-py/tests/measure_sources/nested/orders.py b/pkg-py/tests/measure_sources/nested/orders.py new file mode 100644 index 00000000..a17d26be --- /dev/null +++ b/pkg-py/tests/measure_sources/nested/orders.py @@ -0,0 +1,12 @@ +"""Shares a file name with the parent directory's orders.py on purpose. + +Directory loading must not reach it, and loading it explicitly must not +collide with the other orders.py in sys.modules. +""" + +from commons._measures import measure + + +@measure(description="Count of nested orders.") +def nested_order_count() -> int: + return 1 diff --git a/pkg-py/tests/measure_sources/orders.py b/pkg-py/tests/measure_sources/orders.py new file mode 100644 index 00000000..614fa972 --- /dev/null +++ b/pkg-py/tests/measure_sources/orders.py @@ -0,0 +1,20 @@ +"""Measures loaded from a path by the test suite.""" + +from typing import Annotated, Any + +from pydantic import Field + +from commons._measures import Injected, measure + + +def double(x: int) -> int: + """A helper the measure calls. Not a measure itself.""" + return x * 2 + + +@measure(description="Count of orders.") +def order_count( + region: Annotated[str, Field(description="The sales region.")], + warehouse: Injected[Any], +) -> int: + return double(1) diff --git a/pkg-py/tests/measure_sources/revenue.py b/pkg-py/tests/measure_sources/revenue.py new file mode 100644 index 00000000..78375b02 --- /dev/null +++ b/pkg-py/tests/measure_sources/revenue.py @@ -0,0 +1,8 @@ +"""A second file in the same directory, to prove directory loading.""" + +from commons._measures import measure + + +@measure(description="Total revenue.") +def total_revenue() -> int: + return 100 diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index c6c03afe..c3cdb218 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -1,8 +1,10 @@ """The semantic layer: measures, their schemas, and injected arguments.""" import enum +import importlib from collections.abc import AsyncIterator from dataclasses import FrozenInstanceError +from pathlib import Path from typing import Annotated, Any, Literal, get_args, get_origin import pytest @@ -652,3 +654,81 @@ def test_semantic_layer_reports_its_size() -> None: assert len(layer) == 1 assert "1 measure" in repr(layer) + + +MEASURE_FILES = Path(__file__).parent / "measure_sources" + + +def test_semantic_layer_reads_a_file_path() -> None: + layer = semantic_layer(MEASURE_FILES / "orders.py") + + assert list(layer.measures) == ["order_count"] + + +def test_semantic_layer_accepts_a_string_path() -> None: + layer = semantic_layer(str(MEASURE_FILES / "orders.py")) + + assert list(layer.measures) == ["order_count"] + + +def test_semantic_layer_reads_a_directory_without_recursing() -> None: + layer = semantic_layer(MEASURE_FILES) + + assert list(layer.measures) == ["order_count", "total_revenue"] + + +def test_semantic_layer_reads_a_module_object() -> None: + module = importlib.import_module("commons._measures") + + layer = semantic_layer(module) + + assert layer.measures == {} + + +def test_semantic_layer_mixes_files_and_inline_measures() -> None: + @measure(description="Inline.") + def inline_measure() -> int: + return 1 + + layer = semantic_layer(MEASURE_FILES / "orders.py", inline_measure) + + assert list(layer.measures) == ["order_count", "inline_measure"] + + +def test_semantic_layer_harvests_helper_source_alongside_measures() -> None: + layer = semantic_layer(MEASURE_FILES / "orders.py") + + assert set(layer.source_text) >= {"double", "order_count"} + assert "x * 2" in layer.source_text["double"] + assert "@measure(" in layer.source_text["order_count"] + + +def test_harvested_source_excludes_imported_names() -> None: + layer = semantic_layer(MEASURE_FILES / "orders.py") + + assert "measure" not in layer.source_text + assert "Field" not in layer.source_text + + +def test_only_text_leaves_the_semantic_layer() -> None: + layer = semantic_layer(MEASURE_FILES / "orders.py") + + assert all(isinstance(text, str) for text in layer.source_text.values()) + + +def test_same_file_name_in_two_directories_both_load() -> None: + layer = semantic_layer( + MEASURE_FILES / "orders.py", MEASURE_FILES / "nested" / "orders.py" + ) + + assert list(layer.measures) == ["order_count", "nested_order_count"] + + +def test_missing_path_is_an_error() -> None: + with pytest.raises(ValueError, match="not a measure"): + semantic_layer("not a measure") + + +def test_missing_path_error_names_the_path() -> None: + with pytest.raises(ValueError, match="nowhere.py"): + semantic_layer(MEASURE_FILES / "nowhere.py") From f3d599da869ef08c142858c2afb51510dd44e1e9 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 21:27:39 -0600 Subject: [PATCH 06/19] fix(py): first-definition-wins for all source merges, clean sys.modules on import failure --- pkg-py/src/commons/_measures.py | 32 +++++++++++++------ .../measure_sources/broken/broken_import.py | 6 ++++ .../duplicate_helpers/a_file.py | 13 ++++++++ .../duplicate_helpers/b_file.py | 17 ++++++++++ pkg-py/tests/test_measures.py | 20 ++++++++++++ 5 files changed, 78 insertions(+), 10 deletions(-) create mode 100644 pkg-py/tests/measure_sources/broken/broken_import.py create mode 100644 pkg-py/tests/measure_sources/duplicate_helpers/a_file.py create mode 100644 pkg-py/tests/measure_sources/duplicate_helpers/b_file.py diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index fd4eafe9..7aaa4a8d 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -397,10 +397,7 @@ def semantic_layer(*items: Any) -> SemanticLayer: if record.name in measures: duplicates.append(record.name) measures[record.name] = record - for name, text in sources.items(): - # First definition wins, matching R's de-duplication of harvested - # sources across files. - source_text.setdefault(name, text) + _merge_sources(source_text, sources) if duplicates: raise ValueError( @@ -423,9 +420,7 @@ def _collect(item: Any) -> tuple[list[Measure], dict[str, str]]: for entry in item: found, text = _collect(entry) measures.extend(found) - for name, name_text in text.items(): - # First definition wins, matching semantic_layer()'s rule. - sources.setdefault(name, name_text) + _merge_sources(sources, text) return measures, sources if isinstance(item, ModuleType): @@ -469,10 +464,20 @@ def _from_path(path: Path) -> tuple[list[Measure], dict[str, str]]: for file in files: found, text = _from_module(_load_module_from_path(file)) measures.extend(found) - sources.update(text) + _merge_sources(sources, text) return measures, sources +def _merge_sources(target: dict[str, str], found: Mapping[str, str]) -> None: + """Merge harvested source text; the first definition of a name wins. + + Every place source text is combined across items uses this, so a new + merge point cannot quietly pick the wrong precedence. + """ + for name, text in found.items(): + target.setdefault(name, text) + + def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: """Harvest a module's measures and the source of every function it defines. @@ -501,10 +506,17 @@ def _load_module_from_path(path: Path) -> ModuleType: name = f"commons._measure_sources.{path.stem}_{digest}" spec = importlib.util.spec_from_file_location(name, path) if spec is None or spec.loader is None: - raise ValueError(f"Cannot read measures from {path}: not a Python file.") + raise ValueError( + f"Cannot read measures from {path}: not a Python file.\n" + f"Pass a .py file, a directory of them, or a module object." + ) module = importlib.util.module_from_spec(spec) sys.modules[name] = module - spec.loader.exec_module(module) + try: + spec.loader.exec_module(module) + except BaseException: + del sys.modules[name] + raise return module diff --git a/pkg-py/tests/measure_sources/broken/broken_import.py b/pkg-py/tests/measure_sources/broken/broken_import.py new file mode 100644 index 00000000..653a1100 --- /dev/null +++ b/pkg-py/tests/measure_sources/broken/broken_import.py @@ -0,0 +1,6 @@ +"""Raises at import time, to test that a failed load does not dirty sys.modules. + +Lives in a subdirectory so a non-recursive directory scan never reaches it. +""" + +raise RuntimeError("boom") diff --git a/pkg-py/tests/measure_sources/duplicate_helpers/a_file.py b/pkg-py/tests/measure_sources/duplicate_helpers/a_file.py new file mode 100644 index 00000000..a6358dcf --- /dev/null +++ b/pkg-py/tests/measure_sources/duplicate_helpers/a_file.py @@ -0,0 +1,13 @@ +"""First file, sorted before b_file.py in this directory.""" + +from commons._measures import measure + + +def helper() -> int: + """A helper this file's measure calls.""" + return 1 + + +@measure(description="Measure a.") +def measure_a() -> int: + return helper() diff --git a/pkg-py/tests/measure_sources/duplicate_helpers/b_file.py b/pkg-py/tests/measure_sources/duplicate_helpers/b_file.py new file mode 100644 index 00000000..6734b4cb --- /dev/null +++ b/pkg-py/tests/measure_sources/duplicate_helpers/b_file.py @@ -0,0 +1,17 @@ +"""Second file, sorted after a_file.py; defines a same-named helper. + +Proves directory scanning keeps the first file's source for a colliding +helper name. +""" + +from commons._measures import measure + + +def helper() -> int: + """A colliding helper name; this definition must lose to a_file's.""" + return 2 + + +@measure(description="Measure b.") +def measure_b() -> int: + return helper() diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index c3cdb218..4f776310 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -2,6 +2,7 @@ import enum import importlib +import sys from collections.abc import AsyncIterator from dataclasses import FrozenInstanceError from pathlib import Path @@ -732,3 +733,22 @@ def test_missing_path_is_an_error() -> None: def test_missing_path_error_names_the_path() -> None: with pytest.raises(ValueError, match="nowhere.py"): semantic_layer(MEASURE_FILES / "nowhere.py") + + +def test_directory_scan_keeps_the_first_files_helper_source() -> None: + # a_file.py sorts before b_file.py; both define a `helper` function, and + # the first one scanned must win. + layer = semantic_layer(MEASURE_FILES / "duplicate_helpers") + + assert list(layer.measures) == ["measure_a", "measure_b"] + assert "return 1" in layer.source_text["helper"] + assert "return 2" not in layer.source_text["helper"] + + +def test_failed_import_does_not_dirty_sys_modules() -> None: + path = MEASURE_FILES / "broken" / "broken_import.py" + + with pytest.raises(RuntimeError, match="boom"): + semantic_layer(path) + + assert not any("broken_import" in name for name in sys.modules) From 02a219107dd42b424158b00b81949a89fb6c7f42 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 21:31:41 -0600 Subject: [PATCH 07/19] fix(py): pop, don't del, sys.modules entry on measure-file import failure --- pkg-py/src/commons/_measures.py | 2 +- .../measure_sources/broken/self_removing_import.py | 11 +++++++++++ pkg-py/tests/test_measures.py | 9 +++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 pkg-py/tests/measure_sources/broken/self_removing_import.py diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 7aaa4a8d..29e264f7 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -515,7 +515,7 @@ def _load_module_from_path(path: Path) -> ModuleType: try: spec.loader.exec_module(module) except BaseException: - del sys.modules[name] + sys.modules.pop(name, None) raise return module diff --git a/pkg-py/tests/measure_sources/broken/self_removing_import.py b/pkg-py/tests/measure_sources/broken/self_removing_import.py new file mode 100644 index 00000000..c1ba07ac --- /dev/null +++ b/pkg-py/tests/measure_sources/broken/self_removing_import.py @@ -0,0 +1,11 @@ +"""Deletes its own sys.modules entry, then raises. + +Regression fixture for _load_module_from_path's cleanup: it must not turn +this into a KeyError and swallow the real import error. +""" + +import sys + +del sys.modules[__name__] + +raise RuntimeError("boom") diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index 4f776310..1d84f96a 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -752,3 +752,12 @@ def test_failed_import_does_not_dirty_sys_modules() -> None: semantic_layer(path) assert not any("broken_import" in name for name in sys.modules) + + +def test_failed_import_that_deletes_its_own_module_entry_still_raises() -> None: + # If the module removes its sys.modules entry before raising, cleanup + # must not turn the real error into a KeyError. + path = MEASURE_FILES / "broken" / "self_removing_import.py" + + with pytest.raises(RuntimeError, match="boom"): + semantic_layer(path) From 2da1374119343161a653064a6c8179a97a282c87 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 21:43:18 -0600 Subject: [PATCH 08/19] feat(py): allow sibling-file imports for path-loaded measures, guard against name collisions --- pkg-py/src/commons/_measures.py | 54 +++++++++++++++++-- .../measure_sources/collision_a/shared_lib.py | 5 ++ .../collision_a/uses_shared.py | 12 +++++ .../measure_sources/collision_b/shared_lib.py | 9 ++++ .../sibling_imports/helper_lib.py | 6 +++ .../sibling_imports/uses_helper.py | 12 +++++ .../measure_sources/stdlib_collision/json.py | 10 ++++ pkg-py/tests/test_measures.py | 43 +++++++++++++++ 8 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 pkg-py/tests/measure_sources/collision_a/shared_lib.py create mode 100644 pkg-py/tests/measure_sources/collision_a/uses_shared.py create mode 100644 pkg-py/tests/measure_sources/collision_b/shared_lib.py create mode 100644 pkg-py/tests/measure_sources/sibling_imports/helper_lib.py create mode 100644 pkg-py/tests/measure_sources/sibling_imports/uses_helper.py create mode 100644 pkg-py/tests/measure_sources/stdlib_collision/json.py diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 29e264f7..35c8eecb 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -384,8 +384,10 @@ def semantic_layer(*items: Any) -> SemanticLayer: Each item is a measure, a list of measures, a module, or a path to a Python file or a directory of them. Directory searches are not recursive. - A measure that calls a helper defined in another file imports it, the way - any Python module does. + A sibling file is imported by plain absolute import; its directory is on + sys.path only while the file loads. A file whose name collides with the + standard library, or with a module already imported from elsewhere, is a + construction error. """ measures: dict[str, Measure] = {} source_text: dict[str, str] = {} @@ -510,16 +512,62 @@ def _load_module_from_path(path: Path) -> ModuleType: f"Cannot read measures from {path}: not a Python file.\n" f"Pass a .py file, a directory of them, or a module object." ) + + _check_directory_importable(path.parent) + module = importlib.util.module_from_spec(spec) sys.modules[name] = module + + # Appended, not inserted at the front: a sibling file can then import + # another sibling by plain absolute import, but a sibling named like a + # stdlib module must not shadow it for the rest of the process. + directory = str(path.parent) + added_to_path = directory not in sys.path + if added_to_path: + sys.path.append(directory) try: spec.loader.exec_module(module) except BaseException: - sys.modules.pop(name, None) + if sys.modules.get(name) is module: + sys.modules.pop(name, None) raise + finally: + if added_to_path and directory in sys.path: + sys.path.remove(directory) return module +def _check_directory_importable(directory: Path) -> None: + """Fail before a directory goes on sys.path if a file in it would shadow + the standard library or collide with a module already imported from + elsewhere. + + Every .py file in the directory is checked, not only the one being + loaded: the sys.path entry makes all of them importable, so an unloaded + file with a colliding name is exactly as dangerous. + """ + for entry in sorted(directory.glob("*.py")): + if entry.name == "__init__.py": + continue + stem = entry.stem + if stem in sys.stdlib_module_names: + raise ValueError( + f"{entry} would shadow the standard library module {stem!r} " + f"once its directory is importable.\n" + f"Rename the file." + ) + existing = sys.modules.get(stem) + existing_file = getattr(existing, "__file__", None) if existing else None + if existing is not None and ( + existing_file is None or Path(existing_file).resolve() != entry.resolve() + ): + raise ValueError( + f"{entry} would collide with {stem!r}, already imported " + f"from {existing_file or 'a module with no file'}.\n" + f"Rename the file." + ) + + def _source_text(func: Callable[..., Any]) -> str: try: return inspect.getsource(func) diff --git a/pkg-py/tests/measure_sources/collision_a/shared_lib.py b/pkg-py/tests/measure_sources/collision_a/shared_lib.py new file mode 100644 index 00000000..2ef481e0 --- /dev/null +++ b/pkg-py/tests/measure_sources/collision_a/shared_lib.py @@ -0,0 +1,5 @@ +"""Shares a file name with collision_b/shared_lib.py on purpose.""" + + +def value() -> int: + return 1 diff --git a/pkg-py/tests/measure_sources/collision_a/uses_shared.py b/pkg-py/tests/measure_sources/collision_a/uses_shared.py new file mode 100644 index 00000000..6f48f216 --- /dev/null +++ b/pkg-py/tests/measure_sources/collision_a/uses_shared.py @@ -0,0 +1,12 @@ +"""Imports shared_lib by its bare name, registering it in sys.modules under +that name for the rest of the process. +""" + +from shared_lib import value # type: ignore[missing-import] + +from commons._measures import measure + + +@measure(description="From directory a.") +def a_measure() -> int: + return value() diff --git a/pkg-py/tests/measure_sources/collision_b/shared_lib.py b/pkg-py/tests/measure_sources/collision_b/shared_lib.py new file mode 100644 index 00000000..2ca54b4d --- /dev/null +++ b/pkg-py/tests/measure_sources/collision_b/shared_lib.py @@ -0,0 +1,9 @@ +"""Shares a file name with collision_a/shared_lib.py on purpose. + +Loading anything from this directory must fail once collision_a's +shared_lib.py has already been imported under the bare name "shared_lib". +""" + + +def value() -> int: + return 2 diff --git a/pkg-py/tests/measure_sources/sibling_imports/helper_lib.py b/pkg-py/tests/measure_sources/sibling_imports/helper_lib.py new file mode 100644 index 00000000..ba7b0a3d --- /dev/null +++ b/pkg-py/tests/measure_sources/sibling_imports/helper_lib.py @@ -0,0 +1,6 @@ +"""A helper file a sibling measure file imports directly.""" + + +def double(x: int) -> int: + """Doubles a value; imported by a sibling file, not a measure itself.""" + return x * 2 diff --git a/pkg-py/tests/measure_sources/sibling_imports/uses_helper.py b/pkg-py/tests/measure_sources/sibling_imports/uses_helper.py new file mode 100644 index 00000000..5ca6fe59 --- /dev/null +++ b/pkg-py/tests/measure_sources/sibling_imports/uses_helper.py @@ -0,0 +1,12 @@ +"""Imports a sibling file by plain absolute import, the way an ordinary +Python module does. +""" + +from helper_lib import double # type: ignore[missing-import] + +from commons._measures import measure + + +@measure(description="Doubled count.") +def doubled_count() -> int: + return double(21) diff --git a/pkg-py/tests/measure_sources/stdlib_collision/json.py b/pkg-py/tests/measure_sources/stdlib_collision/json.py new file mode 100644 index 00000000..6fa34b58 --- /dev/null +++ b/pkg-py/tests/measure_sources/stdlib_collision/json.py @@ -0,0 +1,10 @@ +"""Named after a standard library module on purpose: loading this must fail +before the directory ever goes on sys.path. +""" + +from commons._measures import measure + + +@measure(description="Should never load.") +def unreachable_measure() -> int: + return 1 diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index 1d84f96a..d4145ba4 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -761,3 +761,46 @@ def test_failed_import_that_deletes_its_own_module_entry_still_raises() -> None: with pytest.raises(RuntimeError, match="boom"): semantic_layer(path) + + +def test_measure_file_imports_a_sibling_file_directly() -> None: + layer = semantic_layer(MEASURE_FILES / "sibling_imports" / "uses_helper.py") + + assert list(layer.measures) == ["doubled_count"] + assert layer.measures["doubled_count"].func() == 42 + + +def test_sys_path_is_restored_after_a_successful_load() -> None: + directory = str(MEASURE_FILES / "sibling_imports") + + semantic_layer(MEASURE_FILES / "sibling_imports" / "uses_helper.py") + + assert directory not in sys.path + + +def test_sys_path_is_restored_after_a_failing_load() -> None: + directory = str(MEASURE_FILES / "broken") + + with pytest.raises(RuntimeError, match="boom"): + semantic_layer(MEASURE_FILES / "broken" / "broken_import.py") + + assert directory not in sys.path + + +def test_stdlib_name_collision_is_a_construction_error() -> None: + path = MEASURE_FILES / "stdlib_collision" / "json.py" + + with pytest.raises(ValueError, match="json.py") as excinfo: + semantic_layer(path) + + assert "standard library" in str(excinfo.value) + + +def test_same_named_helper_in_two_directories_is_a_construction_error() -> None: + try: + semantic_layer(MEASURE_FILES / "collision_a" / "uses_shared.py") + + with pytest.raises(ValueError, match="shared_lib"): + semantic_layer(MEASURE_FILES / "collision_b" / "shared_lib.py") + finally: + sys.modules.pop("shared_lib", None) From 0aeb13ffbd97a10a993c34329e9e13b29d31556a Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 21:57:22 -0600 Subject: [PATCH 09/19] fix(py): catch installed-but-unimported name collisions, serialize import machinery access --- pkg-py/src/commons/_measures.py | 79 +++++++++++++++++++++++---------- pkg-py/tests/test_measures.py | 29 ++++++++++++ 2 files changed, 84 insertions(+), 24 deletions(-) diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 35c8eecb..922da4fa 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -16,6 +16,7 @@ import inspect import os import sys +import threading from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass from pathlib import Path @@ -499,6 +500,12 @@ def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: return measures, sources +# The import machinery (sys.path, sys.modules) is process-global state, not +# owned by any one SemanticLayer, so concurrent construction must serialize +# around it rather than around the layer itself. +_IMPORT_LOCK = threading.Lock() + + def _load_module_from_path(path: Path) -> ModuleType: # The digest keeps two files with the same stem from overwriting each # other in sys.modules; registering before exec_module() is what lets @@ -513,34 +520,35 @@ def _load_module_from_path(path: Path) -> ModuleType: f"Pass a .py file, a directory of them, or a module object." ) - _check_directory_importable(path.parent) - - module = importlib.util.module_from_spec(spec) - sys.modules[name] = module - - # Appended, not inserted at the front: a sibling file can then import - # another sibling by plain absolute import, but a sibling named like a - # stdlib module must not shadow it for the rest of the process. - directory = str(path.parent) - added_to_path = directory not in sys.path - if added_to_path: - sys.path.append(directory) - try: - spec.loader.exec_module(module) - except BaseException: - if sys.modules.get(name) is module: - sys.modules.pop(name, None) - raise - finally: - if added_to_path and directory in sys.path: - sys.path.remove(directory) - return module + with _IMPORT_LOCK: + _check_directory_importable(path.parent) + + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + + # Appended, not inserted at the front: a sibling file can then + # import another sibling by plain absolute import, but a sibling + # named like a stdlib module must not shadow it for the rest of the + # process. + directory = str(path.parent) + added_to_path = directory not in sys.path + if added_to_path: + sys.path.append(directory) + try: + spec.loader.exec_module(module) + except BaseException: + if sys.modules.get(name) is module: + sys.modules.pop(name, None) + raise + finally: + if added_to_path and directory in sys.path: + sys.path.remove(directory) + return module def _check_directory_importable(directory: Path) -> None: """Fail before a directory goes on sys.path if a file in it would shadow - the standard library or collide with a module already imported from - elsewhere. + an importable module or collide with one already loaded from elsewhere. Every .py file in the directory is checked, not only the one being loaded: the sys.path entry makes all of them importable, so an unloaded @@ -550,12 +558,35 @@ def _check_directory_importable(directory: Path) -> None: if entry.name == "__init__.py": continue stem = entry.stem + if stem in sys.stdlib_module_names: raise ValueError( f"{entry} would shadow the standard library module {stem!r} " f"once its directory is importable.\n" f"Rename the file." ) + + # Must run before the directory joins sys.path: added first, the + # file would resolve to itself and every directory would look + # shadowed. find_spec() also catches a module already cached in + # sys.modules under this name (e.g. by an earlier measure + # directory's sibling import), except when that cached entry has no + # discoverable spec, which find_spec() reports by raising instead of + # returning one; the sys.modules check below catches that case. + try: + spec = importlib.util.find_spec(stem) + except (ImportError, ValueError): + spec = None + if spec is not None and ( + spec.origin is None or Path(spec.origin).resolve() != entry.resolve() + ): + origin_note = f" ({spec.origin})" if spec.origin else "" + raise ValueError( + f"{entry} would be shadowed by the already-importable " + f"module {stem!r}{origin_note}.\n" + f"Rename the file." + ) + existing = sys.modules.get(stem) existing_file = getattr(existing, "__file__", None) if existing else None if existing is not None and ( diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index d4145ba4..fc0405d6 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -804,3 +804,32 @@ def test_same_named_helper_in_two_directories_is_a_construction_error() -> None: semantic_layer(MEASURE_FILES / "collision_b" / "shared_lib.py") finally: sys.modules.pop("shared_lib", None) + + +def test_installed_but_unimported_module_is_a_construction_error( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # A directory on sys.path stands in for an installed package: find_spec() + # can resolve it without anything having imported it yet. + site_packages = tmp_path / "site-packages" + site_packages.mkdir() + (site_packages / "certainly_not_a_measure.py").write_text("VALUE = 1\n") + monkeypatch.syspath_prepend(str(site_packages)) + sys.modules.pop("certainly_not_a_measure", None) + + measures_dir = tmp_path / "measures" + measures_dir.mkdir() + colliding = measures_dir / "certainly_not_a_measure.py" + colliding.write_text( + "from commons._measures import measure\n\n\n" + "@measure(description='d')\n" + "def m() -> int:\n" + " return 1\n" + ) + + with pytest.raises(ValueError, match="certainly_not_a_measure.py") as excinfo: + semantic_layer(colliding) + + message = str(excinfo.value) + assert "certainly_not_a_measure" in message + assert "already-importable" in message From 14bd9dc80e4da1a02f18804dfa5f9ba05336d6e4 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 22:00:48 -0600 Subject: [PATCH 10/19] fix(py): make the import lock reentrant to avoid deadlock on nested semantic_layer() calls --- pkg-py/src/commons/_measures.py | 7 +++-- .../reentrant/composes_a_sibling.py | 17 +++++++++++ pkg-py/tests/test_measures.py | 28 +++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 pkg-py/tests/measure_sources/reentrant/composes_a_sibling.py diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 922da4fa..86e32fa9 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -502,8 +502,11 @@ def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: # The import machinery (sys.path, sys.modules) is process-global state, not # owned by any one SemanticLayer, so concurrent construction must serialize -# around it rather than around the layer itself. -_IMPORT_LOCK = threading.Lock() +# around it rather than around the layer itself. Reentrant, not a plain +# Lock: the lock is held across exec_module(), which runs a measure file's +# top-level code, and that code can itself call semantic_layer() on another +# path, re-entering this same function on the same thread. +_IMPORT_LOCK = threading.RLock() def _load_module_from_path(path: Path) -> ModuleType: diff --git a/pkg-py/tests/measure_sources/reentrant/composes_a_sibling.py b/pkg-py/tests/measure_sources/reentrant/composes_a_sibling.py new file mode 100644 index 00000000..0fd7e0a8 --- /dev/null +++ b/pkg-py/tests/measure_sources/reentrant/composes_a_sibling.py @@ -0,0 +1,17 @@ +"""Calls semantic_layer() on another path during its own import. + +A non-reentrant lock around the import machinery would deadlock here: this +module's own load already holds _IMPORT_LOCK when the line below tries to +acquire it again on the same thread. +""" + +from pathlib import Path + +from commons._measures import measure, semantic_layer + +NESTED_LAYER = semantic_layer(Path(__file__).parent.parent / "nested" / "orders.py") + + +@measure(description="Outer measure.") +def outer_measure() -> int: + return 1 diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index fc0405d6..69a22e1a 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -3,6 +3,7 @@ import enum import importlib import sys +import threading from collections.abc import AsyncIterator from dataclasses import FrozenInstanceError from pathlib import Path @@ -833,3 +834,30 @@ def test_installed_but_unimported_module_is_a_construction_error( message = str(excinfo.value) assert "certainly_not_a_measure" in message assert "already-importable" in message + + +def test_semantic_layer_reenters_during_a_measure_files_import() -> None: + # A non-reentrant lock deadlocks here rather than raising, so this runs + # on a daemon thread with a timeout: a regression fails the test instead + # of hanging the suite. + result: dict[str, Any] = {} + + def target() -> None: + result["layer"] = semantic_layer( + MEASURE_FILES / "reentrant" / "composes_a_sibling.py" + ) + + thread = threading.Thread(target=target, daemon=True) + thread.start() + thread.join(timeout=5) + + assert not thread.is_alive(), ( + "semantic_layer() deadlocked re-entering during a measure file's import" + ) + + outer_layer = result["layer"] + assert list(outer_layer.measures) == ["outer_measure"] + + module_name = outer_layer.measures["outer_measure"].func.__module__ + fixture_module = sys.modules[module_name] + assert list(fixture_module.NESTED_LAYER.measures) == ["nested_order_count"] From 728d70f2d5e56f58a318a747ea9030909110f834 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 22:37:31 -0600 Subject: [PATCH 11/19] fix(py): keep loaded measure files out of the commons namespace _load_module_from_path named every loaded module commons._measure_sources._, so a user's measure file got commons._measure_sources as its __package__: a relative import in their own file failed hunting through commons instead of with Python's own "no known parent package" error, and worse, a name like `.._citations` could resolve into commons' own internals. Switch to a single-segment name with no dotted parent. The digest alone did not make the name unique across repeat loads of the same path, so a second semantic_layer() call on one path clobbered the first load's sys.modules entry, leaving the first layer's Measure.func pointing at a module name that now resolves to the second module. Add a load counter so every load gets its own key; re-execution on repeat loads is unaffected and still matches the R behaviour. Also: fold the stdlib-specific collision check into the general find_spec() check so exactly one error fires per colliding file, and reword its message to state the real direction (stdlib wins, the user's file becomes unreachable, not the reverse); thread the originally requested path through the collision check so a sibling file's collision message says what triggered the scan; move as_measure() and _humanize() next to measure(), which uses them, instead of sitting between unrelated functions; and add a short README to tests/measure_sources/ flagging the two ways that fixture directory silently changes test expectations. --- pkg-py/src/commons/_measures.py | 87 +++++++++++++++----------- pkg-py/tests/measure_sources/README.md | 5 ++ 2 files changed, 55 insertions(+), 37 deletions(-) create mode 100644 pkg-py/tests/measure_sources/README.md diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 86e32fa9..684ff6f0 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -14,6 +14,7 @@ import hashlib import importlib.util import inspect +import itertools import os import sys import threading @@ -257,6 +258,18 @@ def decorate(func: Callable[..., Any]) -> Callable[..., Any]: return decorate +def as_measure(obj: Any) -> Measure | None: + """Recognize a measure, whether decorated function or bare record.""" + if isinstance(obj, Measure): + return obj + record = getattr(obj, MEASURE_ATTRIBUTE, None) + return record if isinstance(record, Measure) else None + + +def _humanize(name: str) -> str: + return name.replace("_", " ") + + def measure_schema_text( record: Measure, source_names: Sequence[str] = (), @@ -346,18 +359,6 @@ def _resolve_ref(node: dict[str, Any], defs: dict[str, Any]) -> dict[str, Any]: return defs[ref.removeprefix("#/$defs/")] -def as_measure(obj: Any) -> Measure | None: - """Recognize a measure, whether decorated function or bare record.""" - if isinstance(obj, Measure): - return obj - record = getattr(obj, MEASURE_ATTRIBUTE, None) - return record if isinstance(record, Measure) else None - - -def _humanize(name: str) -> str: - return name.replace("_", " ") - - @dataclass(frozen=True) class SemanticLayer: """The trusted calculations an agent can run. @@ -508,14 +509,21 @@ def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: # path, re-entering this same function on the same thread. _IMPORT_LOCK = threading.RLock() +# Every load of a path gets its own sys.modules key, even a repeat load of +# the same path: semantic_layer() re-executes a file each time it is passed +# (matching the R behaviour), and each execution needs a key nothing else +# will ever overwrite. +_load_count = itertools.count() + def _load_module_from_path(path: Path) -> ModuleType: - # The digest keeps two files with the same stem from overwriting each - # other in sys.modules; registering before exec_module() is what lets - # dataclasses and typing resolve names back to the module while it is - # still executing. + # A single path segment, not `commons._measure_sources.`: a dotted + # name makes `commons._measure_sources` the loaded file's __package__, + # so a relative import in the user's own file would resolve into + # commons' internals instead of failing with Python's own "no known + # parent package" error. digest = hashlib.sha256(str(path.resolve()).encode()).hexdigest()[:8] - name = f"commons._measure_sources.{path.stem}_{digest}" + name = f"_commons_measure_source_{path.stem}_{digest}_{next(_load_count)}" spec = importlib.util.spec_from_file_location(name, path) if spec is None or spec.loader is None: raise ValueError( @@ -524,7 +532,7 @@ def _load_module_from_path(path: Path) -> ModuleType: ) with _IMPORT_LOCK: - _check_directory_importable(path.parent) + _check_directory_importable(path.parent, requested=path) module = importlib.util.module_from_spec(spec) sys.modules[name] = module @@ -549,30 +557,27 @@ def _load_module_from_path(path: Path) -> ModuleType: return module -def _check_directory_importable(directory: Path) -> None: +def _check_directory_importable(directory: Path, requested: Path) -> None: """Fail before a directory goes on sys.path if a file in it would shadow an importable module or collide with one already loaded from elsewhere. - Every .py file in the directory is checked, not only the one being - loaded: the sys.path entry makes all of them importable, so an unloaded - file with a colliding name is exactly as dangerous. + Every .py file in the directory is checked, not only ``requested``: the + sys.path entry makes all of them importable, so an unloaded file with a + colliding name is exactly as dangerous. ``requested`` is named in every + message so a sibling file's collision is not reported with nothing + connecting it to the file the caller actually asked to load. """ for entry in sorted(directory.glob("*.py")): if entry.name == "__init__.py": continue stem = entry.stem - if stem in sys.stdlib_module_names: - raise ValueError( - f"{entry} would shadow the standard library module {stem!r} " - f"once its directory is importable.\n" - f"Rename the file." - ) - # Must run before the directory joins sys.path: added first, the # file would resolve to itself and every directory would look - # shadowed. find_spec() also catches a module already cached in - # sys.modules under this name (e.g. by an earlier measure + # shadowed. A stdlib name is always findable, so it is folded into + # this check rather than tested separately, keeping exactly one + # raise per entry. find_spec() also catches a module already cached + # in sys.modules under this name (e.g. by an earlier measure # directory's sibling import), except when that cached entry has no # discoverable spec, which find_spec() reports by raising instead of # returning one; the sys.modules check below catches that case. @@ -583,11 +588,18 @@ def _check_directory_importable(directory: Path) -> None: if spec is not None and ( spec.origin is None or Path(spec.origin).resolve() != entry.resolve() ): + if stem in sys.stdlib_module_names: + raise ValueError( + f"While loading {requested}, {entry} collides with the " + f"standard library module {stem!r}; one of the two will " + f"be unreachable.\n" + f"Rename {entry}." + ) origin_note = f" ({spec.origin})" if spec.origin else "" raise ValueError( - f"{entry} would be shadowed by the already-importable " - f"module {stem!r}{origin_note}.\n" - f"Rename the file." + f"While loading {requested}, {entry} would be shadowed by " + f"the already-importable module {stem!r}{origin_note}.\n" + f"Rename {entry}." ) existing = sys.modules.get(stem) @@ -596,9 +608,10 @@ def _check_directory_importable(directory: Path) -> None: existing_file is None or Path(existing_file).resolve() != entry.resolve() ): raise ValueError( - f"{entry} would collide with {stem!r}, already imported " - f"from {existing_file or 'a module with no file'}.\n" - f"Rename the file." + f"While loading {requested}, {entry} collides with {stem!r}, " + f"already imported from " + f"{existing_file or 'a module with no file'}.\n" + f"Rename {entry}." ) diff --git a/pkg-py/tests/measure_sources/README.md b/pkg-py/tests/measure_sources/README.md new file mode 100644 index 00000000..8dd169f6 --- /dev/null +++ b/pkg-py/tests/measure_sources/README.md @@ -0,0 +1,5 @@ +Two traps for the next person editing this directory: the top level is +itself a fixture case, so adding any `.py` file here changes the expected +measure list in `test_semantic_layer_reads_a_directory_without_recursing`; +and the collision check scans every sibling file, so adding a top-level file +named after any importable module breaks every path-loading test at once. From 19a89b8baa09a5cf92f50e2ba2b59faf82e5a3b4 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Wed, 2 Sep 2026 18:13:00 -0600 Subject: [PATCH 12/19] fix(py): cache loaded measure modules by path and mtime instead of a load counter The load counter minted a fresh sys.modules entry on every load, and none was ever removed: repeated semantic_layer(path) calls, e.g. one per agent session, retained every discarded module and everything it held onto. Cache by resolved path and mtime instead, keyed off the path digest alone. A cache hit reuses the existing module and skips re-execution and the directory-collision check entirely; a miss executes and replaces the entry. This bounds sys.modules growth by the number of distinct files loaded rather than the number of loads. The tradeoff: a file edited mid-process now reloads under the same name, so an earlier layer's Measure.func.__module__ mapping resolves to the newer module object -- a development-time scenario, not a leak. Also: sanitize the path stem before building the module name. A dotted filename like sales.q3.py produced a dotted module name even after the single-segment fix, since path.stem for it is "sales.q3", undoing the fix by giving the loaded file a non-empty __package__ again. --- pkg-py/src/commons/_measures.py | 52 ++++++++++++++++++++++++--------- pkg-py/tests/test_measures.py | 15 ++++++++++ 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 684ff6f0..9532cfbe 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -14,8 +14,8 @@ import hashlib import importlib.util import inspect -import itertools import os +import re import sys import threading from collections.abc import Callable, Mapping, Sequence @@ -509,33 +509,56 @@ def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: # path, re-entering this same function on the same thread. _IMPORT_LOCK = threading.RLock() -# Every load of a path gets its own sys.modules key, even a repeat load of -# the same path: semantic_layer() re-executes a file each time it is passed -# (matching the R behaviour), and each execution needs a key nothing else -# will ever overwrite. -_load_count = itertools.count() +# Anything that is not a plain identifier character, including the dots in a +# name like `sales.q3.py`: left alone, a dotted stem would still produce a +# dotted module name, defeating the single-segment name below. +_UNSAFE_NAME_CHARS = re.compile(r"[^0-9a-zA-Z_]") + +# The mtime a path's cached module was loaded at, keyed by module name. +# Compared against the file's current mtime on every load so a module is +# reused only while its source is unchanged; sys.modules alone cannot tell a +# fresh load from a stale one. +_load_mtimes: dict[str, float] = {} def _load_module_from_path(path: Path) -> ModuleType: + resolved = path.resolve() + stem = _UNSAFE_NAME_CHARS.sub("_", path.stem) + # The digest, not a load counter: two semantic_layer() calls on the same + # path should reuse the same module when its source is unchanged, rather + # than each minting a new sys.modules entry the old one is never removed + # from -- an application constructing an agent per session leaked one + # module, and everything it held onto, per session. The cost is that a + # file edited mid-process reloads under the same name, so an earlier + # layer's Measure.func.__module__ then resolves to the newer module + # object; a development-time scenario, not a session-count-scaling leak. + # # A single path segment, not `commons._measure_sources.`: a dotted # name makes `commons._measure_sources` the loaded file's __package__, # so a relative import in the user's own file would resolve into # commons' internals instead of failing with Python's own "no known # parent package" error. - digest = hashlib.sha256(str(path.resolve()).encode()).hexdigest()[:8] - name = f"_commons_measure_source_{path.stem}_{digest}_{next(_load_count)}" - spec = importlib.util.spec_from_file_location(name, path) - if spec is None or spec.loader is None: - raise ValueError( - f"Cannot read measures from {path}: not a Python file.\n" - f"Pass a .py file, a directory of them, or a module object." - ) + digest = hashlib.sha256(str(resolved).encode()).hexdigest()[:8] + name = f"_commons_measure_source_{stem}_{digest}" with _IMPORT_LOCK: + mtime = resolved.stat().st_mtime + cached = sys.modules.get(name) + if cached is not None and _load_mtimes.get(name) == mtime: + return cached + _check_directory_importable(path.parent, requested=path) + spec = importlib.util.spec_from_file_location(name, path) + if spec is None or spec.loader is None: + raise ValueError( + f"Cannot read measures from {path}: not a Python file.\n" + f"Pass a .py file, a directory of them, or a module object." + ) + module = importlib.util.module_from_spec(spec) sys.modules[name] = module + _load_mtimes[name] = mtime # Appended, not inserted at the front: a sibling file can then # import another sibling by plain absolute import, but a sibling @@ -550,6 +573,7 @@ def _load_module_from_path(path: Path) -> ModuleType: except BaseException: if sys.modules.get(name) is module: sys.modules.pop(name, None) + _load_mtimes.pop(name, None) raise finally: if added_to_path and directory in sys.path: diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index 69a22e1a..37e277ae 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -771,6 +771,21 @@ def test_measure_file_imports_a_sibling_file_directly() -> None: assert layer.measures["doubled_count"].func() == 42 +def test_dotted_filename_does_not_get_a_dotted_module_name( + tmp_path: Path, +) -> None: + # path.stem for "sales.q3.py" is "sales.q3": left unsanitized, the + # generated module name would still be dotted, giving the loaded file a + # non-empty __package__ and undoing the single-segment name fix. + dotted = tmp_path / "sales.q3.py" + dotted.write_text("from ..nope import thing\n") + + with pytest.raises( + ImportError, match="attempted relative import with no known parent package" + ): + semantic_layer(dotted) + + def test_sys_path_is_restored_after_a_successful_load() -> None: directory = str(MEASURE_FILES / "sibling_imports") From 5ac659ae6182ceff8d2be671f6859ec18b625821 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Wed, 2 Sep 2026 18:19:19 -0600 Subject: [PATCH 13/19] fix(py): verify file identity on a cache hit, use nanosecond mtimes The cache-hit test compared only mtime, never confirming the cached module was actually this file. The module name carries just the first 8 hex characters of the path digest, so two distinct files with the same sanitized stem can collide on that 32-bit value; if their mtimes also matched, a load of the second file silently returned the first file's module and therefore its measures. Add the missing identity check: reuse a cached module only if its own __file__ also resolves to the requested path. That makes the digest's length irrelevant to correctness, since it is then only a name, and a collision degrades to a reload rather than to wrong measures. A miss on a colliding name still replaces the sys.modules entry rather than erroring, which is safe: nothing depends on that name continuing to point at the other file's module, since a Measure holds its function directly, not a lookup through the module name. Also: switch the recorded load-time mtime to st_mtime_ns. A float st_mtime can lose enough filesystem timestamp precision that two rapid edits look identical and a stale module stays cached. --- pkg-py/src/commons/_measures.py | 40 ++++++++++++++++++----- pkg-py/tests/test_measures.py | 56 +++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 9532cfbe..e2bc6ab9 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -514,11 +514,13 @@ def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: # dotted module name, defeating the single-segment name below. _UNSAFE_NAME_CHARS = re.compile(r"[^0-9a-zA-Z_]") -# The mtime a path's cached module was loaded at, keyed by module name. -# Compared against the file's current mtime on every load so a module is -# reused only while its source is unchanged; sys.modules alone cannot tell a -# fresh load from a stale one. -_load_mtimes: dict[str, float] = {} +# The mtime, in nanoseconds, a path's cached module was loaded at, keyed by +# module name. st_mtime_ns, not st_mtime: a float mtime can lose enough +# filesystem timestamp precision that two rapid edits look identical and a +# stale module stays cached. Compared against the file's current mtime on +# every load so a module is reused only while its source is unchanged; +# sys.modules alone cannot tell a fresh load from a stale one. +_load_mtimes: dict[str, int] = {} def _load_module_from_path(path: Path) -> ModuleType: @@ -542,9 +544,13 @@ def _load_module_from_path(path: Path) -> ModuleType: name = f"_commons_measure_source_{stem}_{digest}" with _IMPORT_LOCK: - mtime = resolved.stat().st_mtime + mtime_ns = resolved.stat().st_mtime_ns cached = sys.modules.get(name) - if cached is not None and _load_mtimes.get(name) == mtime: + if ( + cached is not None + and _load_mtimes.get(name) == mtime_ns + and _cached_module_path(cached) == resolved + ): return cached _check_directory_importable(path.parent, requested=path) @@ -556,9 +562,18 @@ def _load_module_from_path(path: Path) -> ModuleType: f"Pass a .py file, a directory of them, or a module object." ) + # A miss here can mean a stale or wrong-file entry already occupies + # `name`: the digest is only 32 bits, so two distinct files with the + # same sanitized stem can collide on it. Replacing the entry, rather + # than erroring, is safe because nothing depends on sys.modules[name] + # continuing to point at the other file's module -- a Measure holds + # its function directly, not a lookup through this name -- so the + # collision degrades to that other file re-executing on its own next + # load (the identity check above will miss for it too), never to + # this load returning its measures. module = importlib.util.module_from_spec(spec) sys.modules[name] = module - _load_mtimes[name] = mtime + _load_mtimes[name] = mtime_ns # Appended, not inserted at the front: a sibling file can then # import another sibling by plain absolute import, but a sibling @@ -581,6 +596,15 @@ def _load_module_from_path(path: Path) -> ModuleType: return module +def _cached_module_path(module: ModuleType) -> Path | None: + """Resolve a cached module's own file, to confirm a name match is + actually the same file and not a truncated-digest collision between two + different ones. + """ + file = getattr(module, "__file__", None) + return Path(file).resolve() if file else None + + def _check_directory_importable(directory: Path, requested: Path) -> None: """Fail before a directory goes on sys.path if a file in it would shadow an importable module or collide with one already loaded from elsewhere. diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index 37e277ae..dc2d5540 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -2,11 +2,13 @@ import enum import importlib +import os import sys import threading from collections.abc import AsyncIterator from dataclasses import FrozenInstanceError from pathlib import Path +from types import ModuleType from typing import Annotated, Any, Literal, get_args, get_origin import pytest @@ -16,6 +18,7 @@ INJECTED, Injected, Measure, + _load_module_from_path, _split_parameters, as_measure, measure, @@ -786,6 +789,59 @@ def test_dotted_filename_does_not_get_a_dotted_module_name( semantic_layer(dotted) +def test_load_module_from_path_reuses_an_unchanged_file(tmp_path: Path) -> None: + source = tmp_path / "m.py" + source.write_text("VALUE = 1\n") + + first = _load_module_from_path(source) + second = _load_module_from_path(source) + + assert first is second + + +def test_load_module_from_path_reloads_an_edited_file(tmp_path: Path) -> None: + source = tmp_path / "m.py" + source.write_text("VALUE = 1\n") + first = _load_module_from_path(source) + + source.write_text("VALUE = 2\n") + # Python's own bytecode cache invalidates on a whole-second-truncated + # mtime, not the nanosecond one _load_mtimes compares against; bump by + # whole seconds so the .pyc it writes on the first load is not reused + # for the second, which would otherwise return stale content regardless + # of what our own cache decides. + stat = source.stat() + os.utime(source, ns=(stat.st_atime_ns, stat.st_mtime_ns + 2_000_000_000)) + + second = _load_module_from_path(source) + + assert second is not first + assert second.VALUE == 2 + + +def test_load_module_from_path_ignores_a_same_named_module_from_elsewhere( + tmp_path: Path, +) -> None: + # Exercises the identity check directly rather than forcing a genuine + # 32-bit digest collision between two distinct filenames: plant a module + # under the exact sys.modules name this path would use, with the same + # recorded mtime but a __file__ pointing elsewhere, and confirm the real + # file is (re-)loaded rather than the stand-in being returned. + source = tmp_path / "m.py" + source.write_text("VALUE = 1\n") + real = _load_module_from_path(source) + name = real.__name__ + + imposter = ModuleType(name) + imposter.__file__ = str(tmp_path / "elsewhere.py") + sys.modules[name] = imposter + + loaded = _load_module_from_path(source) + + assert loaded is not imposter + assert loaded.VALUE == 1 + + def test_sys_path_is_restored_after_a_successful_load() -> None: directory = str(MEASURE_FILES / "sibling_imports") From 57897c1d961c6b24130cdb8636fae74fe593182e Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Wed, 2 Sep 2026 18:24:45 -0600 Subject: [PATCH 14/19] fix(py): invalidate the stale bytecode cache on a detected reload st_mtime_ns fixed our own cache's staleness detection but not SourceFileLoader's: it validates its .pyc by whole-second mtime and size, coarser than what we compare against. An edit within the same second that leaves the file's size unchanged (changing one digit, say) is exactly the case our cache detects and Python's own bytecode cache does not, so the reload ran exec_module() against a module we correctly decided to re-execute, and SourceFileLoader silently handed back the stale compiled code anyway -- worse than staleness, since it looks like a successful reload and returns the wrong answer. Remove the file's own .pyc via importlib.util.cache_from_source() right before re-executing, and only then: on a first load there is nothing stale to invalidate. Best-effort and scoped to the one file being reloaded, since __pycache__ entries are disposable but a permission error removing one should not block loading. Replaced the test's two-second mtime workaround with the real case: same whole second, same file size, pinned explicitly rather than read off the file and nudged so the test cannot straddle a real second boundary and become flaky. --- pkg-py/src/commons/_measures.py | 26 ++++++++++++++++++++++++++ pkg-py/tests/test_measures.py | 17 ++++++++++------- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index e2bc6ab9..90107dda 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -553,6 +553,13 @@ def _load_module_from_path(path: Path) -> ModuleType: ): return cached + if name in _load_mtimes: + # Not this name's first load: the file is being (re)executed + # because the checks above missed. See + # _invalidate_bytecode_cache for why this step is required, not + # just belt-and-suspenders. + _invalidate_bytecode_cache(path) + _check_directory_importable(path.parent, requested=path) spec = importlib.util.spec_from_file_location(name, path) @@ -605,6 +612,25 @@ def _cached_module_path(module: ModuleType) -> Path | None: return Path(file).resolve() if file else None +def _invalidate_bytecode_cache(path: Path) -> None: + """Remove one file's compiled cache before it is re-executed. + + SourceFileLoader validates its own .pyc by whole-second mtime and size, + coarser than the nanosecond mtime this module's cache compares against. + An edit within the same second that leaves the file's size unchanged -- + changing one digit, say -- is exactly the case our cache detects and + SourceFileLoader does not: left alone, it hands back the stale compiled + code and the reload silently runs the old version. Best-effort and + scoped to this one file: the cache directory may be read-only or + already gone, and __pycache__ is disposable by design, but only this + file's entry is touched. + """ + try: + Path(importlib.util.cache_from_source(str(path))).unlink(missing_ok=True) + except (OSError, ValueError, NotImplementedError): + pass + + def _check_directory_importable(directory: Path, requested: Path) -> None: """Fail before a directory goes on sys.path if a file in it would shadow an importable module or collide with one already loaded from elsewhere. diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index dc2d5540..cc54a48e 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -800,18 +800,21 @@ def test_load_module_from_path_reuses_an_unchanged_file(tmp_path: Path) -> None: def test_load_module_from_path_reloads_an_edited_file(tmp_path: Path) -> None: + # An edit within the same whole second that leaves the file's size + # unchanged ("VALUE = 1" -> "VALUE = 2"): the case SourceFileLoader's own + # bytecode cache cannot detect, since it validates by whole-second mtime + # and size, coarser than the nanosecond mtime our cache compares + # against. Both mtimes are pinned explicitly, not read off the file + # naturally and nudged, so the test cannot straddle a real second + # boundary and become flaky. source = tmp_path / "m.py" + base_ns = 1_700_000_000 * 1_000_000_000 source.write_text("VALUE = 1\n") + os.utime(source, ns=(base_ns, base_ns)) first = _load_module_from_path(source) source.write_text("VALUE = 2\n") - # Python's own bytecode cache invalidates on a whole-second-truncated - # mtime, not the nanosecond one _load_mtimes compares against; bump by - # whole seconds so the .pyc it writes on the first load is not reused - # for the second, which would otherwise return stale content regardless - # of what our own cache decides. - stat = source.stat() - os.utime(source, ns=(stat.st_atime_ns, stat.st_mtime_ns + 2_000_000_000)) + os.utime(source, ns=(base_ns, base_ns + 500_000_000)) second = _load_module_from_path(source) From a664efe742d6fe1c5dcfedb2855dd69ca2324bcb Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Wed, 2 Sep 2026 18:29:19 -0600 Subject: [PATCH 15/19] fix(py): invalidate the bytecode cache unconditionally, not only on a detected reload Gating _invalidate_bytecode_cache() on name in _load_mtimes only closed the case where this process had already loaded the file. A stale, timestamp-valid .pyc can also predate this process entirely: an earlier process writes it, the file is edited same-second same-size, and a new process's first load of it has no _load_mtimes entry to have noticed anything, so nothing invalidates and SourceFileLoader runs the old bytecode. Same silent wrong answer as before, reached on a first load instead of a reload. Drop the condition and invalidate before every execution. This removes a branch and a state distinction rather than adding one; the cost is recompiling small measure files at construction time, which is rare and cheap given how rarely those files change while any given process is running. The identity check, the st_mtime_ns comparison, and the tolerant error handling around the unlink are unchanged. --- pkg-py/src/commons/_measures.py | 39 +++++++++++++++++++-------------- pkg-py/tests/test_measures.py | 30 +++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 17 deletions(-) diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 90107dda..f30a89bf 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -553,12 +553,13 @@ def _load_module_from_path(path: Path) -> ModuleType: ): return cached - if name in _load_mtimes: - # Not this name's first load: the file is being (re)executed - # because the checks above missed. See - # _invalidate_bytecode_cache for why this step is required, not - # just belt-and-suspenders. - _invalidate_bytecode_cache(path) + # Unconditional, not just on a detected reload: an earlier process + # can have already written this file's .pyc, and a same-second, + # same-size edit since then leaves it looking valid to + # SourceFileLoader on this process's first load too, which has no + # _load_mtimes entry to have noticed the edit itself. See + # _invalidate_bytecode_cache for why this step is required at all. + _invalidate_bytecode_cache(path) _check_directory_importable(path.parent, requested=path) @@ -613,17 +614,21 @@ def _cached_module_path(module: ModuleType) -> Path | None: def _invalidate_bytecode_cache(path: Path) -> None: - """Remove one file's compiled cache before it is re-executed. - - SourceFileLoader validates its own .pyc by whole-second mtime and size, - coarser than the nanosecond mtime this module's cache compares against. - An edit within the same second that leaves the file's size unchanged -- - changing one digit, say -- is exactly the case our cache detects and - SourceFileLoader does not: left alone, it hands back the stale compiled - code and the reload silently runs the old version. Best-effort and - scoped to this one file: the cache directory may be read-only or - already gone, and __pycache__ is disposable by design, but only this - file's entry is touched. + """Remove one file's compiled cache before executing it. + + Called on every execution, not only a detected reload: a stale, + timestamp-valid .pyc can predate this process entirely, written by an + earlier one. SourceFileLoader validates its own .pyc by whole-second + mtime and size, coarser than the nanosecond mtime this module's cache + compares against; an edit within the same second that leaves the file's + size unchanged (changing one digit, say) is exactly the case + SourceFileLoader cannot detect, whether this is a reload this process + already knows about or a first load of a file some other process + touched. Left uninvalidated, it hands back the stale compiled code and + the load silently runs the old version. Best-effort and scoped to this + one file: the cache directory may be read-only or already gone, and + __pycache__ is disposable by design, but only this file's entry is + touched. """ try: Path(importlib.util.cache_from_source(str(path))).unlink(missing_ok=True) diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index cc54a48e..7445faea 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -19,6 +19,7 @@ Injected, Measure, _load_module_from_path, + _load_mtimes, _split_parameters, as_measure, measure, @@ -822,6 +823,35 @@ def test_load_module_from_path_reloads_an_edited_file(tmp_path: Path) -> None: assert second.VALUE == 2 +def test_load_module_from_path_invalidates_a_pre_existing_bytecode_cache( + tmp_path: Path, +) -> None: + # A stale, timestamp-valid .pyc can predate this process's own record of + # having loaded the file at all -- written by an earlier process, then + # the file edited same-second, same-size before this process's first + # load of it. Simulated here without spawning a real second process: load + # once to produce the .pyc via SourceFileLoader, edit the file, then + # clear this process's own sys.modules and _load_mtimes entries for it + # so the next load has no in-memory record either -- indistinguishable, + # from _load_module_from_path's point of view, from a fresh process's + # first load of an already-edited file. + source = tmp_path / "m.py" + base_ns = 1_700_000_000 * 1_000_000_000 + source.write_text("VALUE = 1\n") + os.utime(source, ns=(base_ns, base_ns)) + first = _load_module_from_path(source) + name = first.__name__ + + source.write_text("VALUE = 2\n") + os.utime(source, ns=(base_ns, base_ns + 500_000_000)) + sys.modules.pop(name, None) + _load_mtimes.pop(name, None) + + second = _load_module_from_path(source) + + assert second.VALUE == 2 + + def test_load_module_from_path_ignores_a_same_named_module_from_elsewhere( tmp_path: Path, ) -> None: From 24c84b6c15e5bebe132b4fcf2e653b04e4f18aba Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Thu, 3 Sep 2026 23:39:54 -0600 Subject: [PATCH 16/19] fix(py): review follow-ups for semantic_layer() collection - the directory collision check no longer feeds dotted or dunder stems to find_spec(), which imported the parent package as a side effect of the read-only check and reported phantom shadowing for names like email.mime.py; subdirectories are checked too, since a sys.path entry makes them importable as packages - collecting the same measure twice (the same function, a file and the directory containing it, an in-module alias) is no longer a duplicate-name error; only a different measure claiming the name is - _from_module() harvests a bare Measure record at module level, not only decorated functions - SemanticLayer gains __iter__ and __contains__ alongside __len__ - tests for the non-.py path error, __init__.py skipping, the source-unavailable fallback, a pre-seeded sys.path entry, and cross-file duplicate measure names --- pkg-py/src/commons/_measures.py | 109 +++++++++++++---- pkg-py/tests/measure_sources/README.md | 6 +- .../tests/measure_sources/aliased/aliased.py | 11 ++ .../measure_sources/bare_record/total.py | 18 +++ .../measure_sources/dotted/email.mime.py | 6 + .../duplicate_measures/a_measure.py | 6 + .../duplicate_measures/b_measure.py | 6 + .../measure_sources/has_init/__init__.py | 4 + .../tests/measure_sources/has_init/orders.py | 6 + .../pkg_collision/json/data.txt | 2 + .../measure_sources/pkg_collision/orders.py | 6 + pkg-py/tests/test_measures.py | 113 +++++++++++++++++- 12 files changed, 263 insertions(+), 30 deletions(-) create mode 100644 pkg-py/tests/measure_sources/aliased/aliased.py create mode 100644 pkg-py/tests/measure_sources/bare_record/total.py create mode 100644 pkg-py/tests/measure_sources/dotted/email.mime.py create mode 100644 pkg-py/tests/measure_sources/duplicate_measures/a_measure.py create mode 100644 pkg-py/tests/measure_sources/duplicate_measures/b_measure.py create mode 100644 pkg-py/tests/measure_sources/has_init/__init__.py create mode 100644 pkg-py/tests/measure_sources/has_init/orders.py create mode 100644 pkg-py/tests/measure_sources/pkg_collision/json/data.txt create mode 100644 pkg-py/tests/measure_sources/pkg_collision/orders.py diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index f30a89bf..16b302ab 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -18,7 +18,7 @@ import re import sys import threading -from collections.abc import Callable, Mapping, Sequence +from collections.abc import Callable, Iterator, Mapping, Sequence from dataclasses import dataclass from pathlib import Path from types import MappingProxyType, ModuleType @@ -228,6 +228,10 @@ def measure( they call stay ordinary callables. Model-supplied arguments are expected to be scalars, enums, or arrays of those; richer shapes are not rejected, but the schema block renders them only approximately. + + ``provenance`` records links back to wherever the measure's definition + came from. The R implementation attaches provenance through a roxygen + tag instead, and only to measures sourced from a file. """ def decorate(func: Callable[..., Any]) -> Callable[..., Any]: @@ -366,6 +370,11 @@ class SemanticLayer: ``source_text`` holds the source of the measures and the module-level helpers they call, keyed by Python name. Only text is kept: the agent's worker session reads measure definitions but never receives a callable. + Two functions that share a Python name share one entry, and the first + definition collected wins, so a measure whose function shares its name + with an earlier one is shown that earlier function's source instead. + + The layout of this object is internal and may change without notice. """ measures: Mapping[str, Measure] @@ -374,6 +383,12 @@ class SemanticLayer: def __len__(self) -> int: return len(self.measures) + def __iter__(self) -> Iterator[str]: + return iter(self.measures) + + def __contains__(self, name: object) -> bool: + return name in self.measures + def __repr__(self) -> str: count = len(self.measures) plural = "" if count == 1 else "s" @@ -383,13 +398,23 @@ def __repr__(self) -> str: def semantic_layer(*items: Any) -> SemanticLayer: """Collect measures into a semantic layer. - Each item is a measure, a list of measures, a module, or a path to a - Python file or a directory of them. Directory searches are not recursive. - - A sibling file is imported by plain absolute import; its directory is on - sys.path only while the file loads. A file whose name collides with the - standard library, or with a module already imported from elsewhere, is a - construction error. + Each item is a measure, a list (or tuple) of measures, a module, or a + path to a Python file or a directory of them. Directory searches are + not recursive. + + A sibling file is imported by plain absolute import; its directory is + on sys.path only while the file loads. Even a single requested file + puts its whole directory on sys.path, so every .py file beside it -- + and every subdirectory, which is importable as a package -- is checked: + a name that collides with the standard library, an installed package, + or a module already imported from elsewhere is a construction error. A + sibling imported this way stays in sys.modules under its bare name for + the rest of the process, so two directories that each define a + same-named helper cannot both be loaded in one process. + + Collecting the same measure twice -- the same file passed alongside + its own directory, say -- is not an error; two different measures + sharing one name is. """ measures: dict[str, Measure] = {} source_text: dict[str, str] = {} @@ -398,7 +423,12 @@ def semantic_layer(*items: Any) -> SemanticLayer: for item in items: found, sources = _collect(item) for record in found: - if record.name in measures: + existing = measures.get(record.name) + # The same record collected again -- the same function twice, a + # file and the directory containing it, an in-module alias -- is + # one measure, not a collision; only a *different* measure + # claiming the name is an error. + if existing is not None and existing is not record: duplicates.append(record.name) measures[record.name] = record _merge_sources(source_text, sources) @@ -436,8 +466,8 @@ def _collect(item: Any) -> tuple[list[Measure], dict[str, str]]: record = as_measure(item) if record is None: raise TypeError( - f"Every item in semantic_layer() must be a measure, a list of " - f"measures, a module, or a path; got {item!r}.\n" + f"Every item in semantic_layer() must be a measure, a list or " + f"tuple of measures, a module, or a path; got {item!r}.\n" f"Decorate the function with @measure to make it one." ) return [record], {record.func.__name__: _source_text(record.func)} @@ -492,11 +522,23 @@ def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: measures: list[Measure] = [] sources: dict[str, str] = {} for name, value in vars(module).items(): - if not inspect.isfunction(value) or value.__module__ != module.__name__: - continue - sources[name] = _source_text(value) - record = as_measure(value) - if record is not None: + if inspect.isfunction(value): + if value.__module__ != module.__name__: + continue + sources[name] = _source_text(value) + record = as_measure(value) + else: + # A measure can also sit at module level as a bare record. One + # wrapping a function from elsewhere is an imported name and + # belongs to the module that defined it, same as any other + # import. + record = as_measure(value) + if record is None: + continue + if getattr(record.func, "__module__", None) != module.__name__: + continue + sources.setdefault(record.func.__name__, _source_text(record.func)) + if record is not None and all(record is not seen for seen in measures): measures.append(record) return measures, sources @@ -506,7 +548,10 @@ def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: # around it rather than around the layer itself. Reentrant, not a plain # Lock: the lock is held across exec_module(), which runs a measure file's # top-level code, and that code can itself call semantic_layer() on another -# path, re-entering this same function on the same thread. +# path, re-entering this same function on the same thread. The flip side of +# holding it across user code: a measure file whose import joins a *thread* +# that constructs a layer deadlocks, because the lock stays owned by the +# importing thread. _IMPORT_LOCK = threading.RLock() # Anything that is not a plain identifier character, including the dots in a @@ -642,14 +687,32 @@ def _check_directory_importable(directory: Path, requested: Path) -> None: Every .py file in the directory is checked, not only ``requested``: the sys.path entry makes all of them importable, so an unloaded file with a - colliding name is exactly as dangerous. ``requested`` is named in every - message so a sibling file's collision is not reported with nothing - connecting it to the file the caller actually asked to load. + colliding name is exactly as dangerous. Subdirectories are checked too: + each is importable as a package, or as a namespace package with no + __init__.py at all. ``requested`` is named in every message so a + sibling's collision is not reported with nothing connecting it to the + file the caller actually asked to load. """ - for entry in sorted(directory.glob("*.py")): - if entry.name == "__init__.py": + for entry in sorted(directory.iterdir()): + if entry.is_file(): + if entry.suffix != ".py" or entry.name == "__init__.py": + continue + stem = entry.stem + elif entry.is_dir(): + stem = entry.name + else: + continue + + # A name that is not a plain identifier -- the dots in + # `sales.q3.py`, a dunder like __pycache__ -- cannot be imported by + # that name, so it can shadow nothing, and looking it up would be + # worse than skipping it: find_spec() on a dotted name imports the + # parent package as a side effect of this read-only check, then + # either raises (swallowed below, silently skipping the check) or + # resolves to an unrelated submodule and reports a phantom + # collision. + if not stem.isidentifier() or (stem.startswith("__") and stem.endswith("__")): continue - stem = entry.stem # Must run before the directory joins sys.path: added first, the # file would resolve to itself and every directory would look diff --git a/pkg-py/tests/measure_sources/README.md b/pkg-py/tests/measure_sources/README.md index 8dd169f6..16142cba 100644 --- a/pkg-py/tests/measure_sources/README.md +++ b/pkg-py/tests/measure_sources/README.md @@ -1,5 +1 @@ -Two traps for the next person editing this directory: the top level is -itself a fixture case, so adding any `.py` file here changes the expected -measure list in `test_semantic_layer_reads_a_directory_without_recursing`; -and the collision check scans every sibling file, so adding a top-level file -named after any importable module breaks every path-loading test at once. +Two traps for the next person editing this directory: the top level is itself a fixture case, so adding any `.py` file here changes the expected measure list in `test_semantic_layer_reads_a_directory_without_recursing`; and the collision check scans every sibling file and subdirectory, so adding a top-level file or directory named after any importable module breaks every path-loading test at once. diff --git a/pkg-py/tests/measure_sources/aliased/aliased.py b/pkg-py/tests/measure_sources/aliased/aliased.py new file mode 100644 index 00000000..bf58cf37 --- /dev/null +++ b/pkg-py/tests/measure_sources/aliased/aliased.py @@ -0,0 +1,11 @@ +from commons._measures import measure + + +@measure(description="Count of orders.") +def aliased_measure() -> int: + return 1 + + +# Re-exporting a measure under a second name in the same module is one +# measure, not a name collision. +also_known_as = aliased_measure diff --git a/pkg-py/tests/measure_sources/bare_record/total.py b/pkg-py/tests/measure_sources/bare_record/total.py new file mode 100644 index 00000000..06817613 --- /dev/null +++ b/pkg-py/tests/measure_sources/bare_record/total.py @@ -0,0 +1,18 @@ +from pydantic import create_model + +from commons._measures import Measure + + +def total() -> int: + return 1 + + +# A measure need not be a decorated function; a bare record at module level +# is harvested too. +grand_total = Measure( + name="grand_total", + title="Grand total", + description="Total of everything.", + func=total, + params=create_model("grand_total"), +) diff --git a/pkg-py/tests/measure_sources/dotted/email.mime.py b/pkg-py/tests/measure_sources/dotted/email.mime.py new file mode 100644 index 00000000..7add29a9 --- /dev/null +++ b/pkg-py/tests/measure_sources/dotted/email.mime.py @@ -0,0 +1,6 @@ +from commons._measures import measure + + +@measure(description="A measure in a file whose name carries a dot.") +def dotted_measure() -> int: + return 1 diff --git a/pkg-py/tests/measure_sources/duplicate_measures/a_measure.py b/pkg-py/tests/measure_sources/duplicate_measures/a_measure.py new file mode 100644 index 00000000..5e6ef41c --- /dev/null +++ b/pkg-py/tests/measure_sources/duplicate_measures/a_measure.py @@ -0,0 +1,6 @@ +from commons._measures import measure + + +@measure(description="First of two distinct measures named dup.", name="dup") +def dup_from_a() -> int: + return 1 diff --git a/pkg-py/tests/measure_sources/duplicate_measures/b_measure.py b/pkg-py/tests/measure_sources/duplicate_measures/b_measure.py new file mode 100644 index 00000000..a0546796 --- /dev/null +++ b/pkg-py/tests/measure_sources/duplicate_measures/b_measure.py @@ -0,0 +1,6 @@ +from commons._measures import measure + + +@measure(description="Second of two distinct measures named dup.", name="dup") +def dup_from_b() -> int: + return 2 diff --git a/pkg-py/tests/measure_sources/has_init/__init__.py b/pkg-py/tests/measure_sources/has_init/__init__.py new file mode 100644 index 00000000..850cbf4b --- /dev/null +++ b/pkg-py/tests/measure_sources/has_init/__init__.py @@ -0,0 +1,4 @@ +raise RuntimeError( + "__init__.py in a measure directory must never be imported; " + "a directory of measure files is not a package." +) diff --git a/pkg-py/tests/measure_sources/has_init/orders.py b/pkg-py/tests/measure_sources/has_init/orders.py new file mode 100644 index 00000000..1f5cff7b --- /dev/null +++ b/pkg-py/tests/measure_sources/has_init/orders.py @@ -0,0 +1,6 @@ +from commons._measures import measure + + +@measure(description="Count of orders.") +def has_init_measure() -> int: + return 1 diff --git a/pkg-py/tests/measure_sources/pkg_collision/json/data.txt b/pkg-py/tests/measure_sources/pkg_collision/json/data.txt new file mode 100644 index 00000000..f11ba53b --- /dev/null +++ b/pkg-py/tests/measure_sources/pkg_collision/json/data.txt @@ -0,0 +1,2 @@ +Stand-in so this otherwise-empty directory is tracked: json/ is importable +as a namespace package once its parent directory is on sys.path. diff --git a/pkg-py/tests/measure_sources/pkg_collision/orders.py b/pkg-py/tests/measure_sources/pkg_collision/orders.py new file mode 100644 index 00000000..6598ab5c --- /dev/null +++ b/pkg-py/tests/measure_sources/pkg_collision/orders.py @@ -0,0 +1,6 @@ +from commons._measures import measure + + +@measure(description="Count of orders.") +def pkg_collision_measure() -> int: + return 1 diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index 7445faea..1b4acc2f 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -610,12 +610,27 @@ def helper() -> int: def test_semantic_layer_rejects_duplicate_names() -> None: + # Two *different* measures that share a name: each factory call decorates + # a fresh function, so the records are distinct objects. + def make() -> Measure: + @measure(description="Count of orders.", name="order_count") + def calc() -> int: + return 1 + + return _as_measure(calc) + + with pytest.raises(ValueError, match="order_count"): + semantic_layer(make(), make()) + + +def test_semantic_layer_accepts_the_same_measure_twice() -> None: @measure(description="Count of orders.") def order_count() -> int: return 1 - with pytest.raises(ValueError, match="order_count"): - semantic_layer(order_count, order_count) + layer = semantic_layer(order_count, order_count) + + assert list(layer.measures) == ["order_count"] def test_semantic_layer_harvests_inline_measure_source() -> None: @@ -965,3 +980,97 @@ def target() -> None: module_name = outer_layer.measures["outer_measure"].func.__module__ fixture_module = sys.modules[module_name] assert list(fixture_module.NESTED_LAYER.measures) == ["nested_order_count"] + + +def test_semantic_layer_supports_membership_and_iteration() -> None: + layer = semantic_layer(_count_measure()) + + assert "order_count" in layer + assert "other" not in layer + assert list(layer) == ["order_count"] + + +def test_a_directory_and_a_file_inside_it_overlap_without_error() -> None: + layer = semantic_layer( + MEASURE_FILES / "sibling_imports", + MEASURE_FILES / "sibling_imports" / "uses_helper.py", + ) + + assert list(layer.measures) == ["doubled_count"] + + +def test_a_module_level_alias_is_collected_once() -> None: + layer = semantic_layer(MEASURE_FILES / "aliased" / "aliased.py") + + assert list(layer.measures) == ["aliased_measure"] + + +def test_distinct_measures_from_two_files_sharing_a_name_are_an_error() -> None: + with pytest.raises(ValueError, match="dup"): + semantic_layer( + MEASURE_FILES / "duplicate_measures" / "a_measure.py", + MEASURE_FILES / "duplicate_measures" / "b_measure.py", + ) + + +def test_a_non_python_file_path_is_an_error(tmp_path: Path) -> None: + notes = tmp_path / "notes.txt" + notes.write_text("not python\n") + + with pytest.raises(ValueError, match="not a Python file"): + semantic_layer(notes) + + +def test_a_directorys_init_py_is_never_imported() -> None: + # has_init/__init__.py raises if it is ever executed. + layer = semantic_layer(MEASURE_FILES / "has_init") + + assert list(layer.measures) == ["has_init_measure"] + + +def test_source_text_falls_back_when_source_is_unavailable() -> None: + # A function built by exec has no file for inspect.getsource to read. + namespace: dict[str, Any] = {} + exec("def ghost() -> int:\n return 1\n", namespace) # noqa: S102 + record = _as_measure(measure(description="d")(namespace["ghost"])) + + layer = semantic_layer(record) + + assert layer.source_text["ghost"] == "# source unavailable for ghost" + + +def test_a_directory_already_on_sys_path_is_left_in_place( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + source = tmp_path / "m.py" + source.write_text("VALUE = 1\n") + monkeypatch.syspath_prepend(str(tmp_path)) + + module = _load_module_from_path(source) + + assert module.VALUE == 1 + assert str(tmp_path) in sys.path + + +def test_a_dotted_filename_loads_despite_the_collision_check() -> None: + # "email.mime" resolves to a real stdlib submodule; the check must skip + # a stem that cannot be imported as a single segment rather than report + # a phantom shadowing (or import the parent package as a side effect). + layer = semantic_layer(MEASURE_FILES / "dotted" / "email.mime.py") + + assert list(layer.measures) == ["dotted_measure"] + + +def test_a_subdirectory_shadowing_the_standard_library_is_an_error() -> None: + # pkg_collision/json/ is importable as a namespace package once its + # parent is on sys.path, exactly like a json.py sibling. + with pytest.raises(ValueError, match="standard library"): + semantic_layer(MEASURE_FILES / "pkg_collision" / "orders.py") + + +def test_a_module_level_bare_record_is_harvested() -> None: + layer = semantic_layer(MEASURE_FILES / "bare_record" / "total.py") + + assert list(layer.measures) == ["grand_total"] + assert layer.measures["grand_total"].func() == 1 + assert "def total" in layer.source_text["total"] From c66c0ad83c930fb0f01ff4600b675dc732a5f6c2 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Fri, 4 Sep 2026 10:22:06 -0600 Subject: [PATCH 17/19] fix(py): review follow-ups for measure collection - Reject a non-.py single-file path up front: spec_from_file_location gives a .pyc or .so a real loader, so such a file executed as bytecode or native code instead of raising "not a Python file". The requested file's error now also wins over a sibling's collision, which was checked before the loader was chosen. - Check a directory's importability once, before anything in it executes, instead of once per file loaded from it. - Key module-harvested source text by function name, not binding, so an in-module alias shares one source_text entry, matching how directly-passed measures are keyed. - Test the spec-less sys.modules collision fallback, the imported bare-record guard, and that the layer's mappings reject mutation. - Docstrings now note where the R implementation differs: it rejects a repeated name even for the same measure twice, and a same-named helper resolves last-wins in its shared source environment. --- pkg-py/src/commons/_measures.py | 65 +++++++++++++++++++-------- pkg-py/tests/test_measures.py | 80 +++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 18 deletions(-) diff --git a/pkg-py/src/commons/_measures.py b/pkg-py/src/commons/_measures.py index 16b302ab..3f2cf85f 100644 --- a/pkg-py/src/commons/_measures.py +++ b/pkg-py/src/commons/_measures.py @@ -368,11 +368,14 @@ class SemanticLayer: """The trusted calculations an agent can run. ``source_text`` holds the source of the measures and the module-level - helpers they call, keyed by Python name. Only text is kept: the agent's - worker session reads measure definitions but never receives a callable. - Two functions that share a Python name share one entry, and the first - definition collected wins, so a measure whose function shares its name - with an earlier one is shown that earlier function's source instead. + helpers they call, keyed by Python function name. Only text is kept: + the agent's worker session reads measure definitions but never receives + a callable. Two functions that share a Python name share one entry, and + the first definition collected wins, so a measure whose function shares + its name with an earlier one is shown that earlier function's source + instead. The R implementation sources a path's files into one shared + environment, so there the last definition of a same-named helper wins + instead, and is what every measure calling it actually runs. The layout of this object is internal and may change without notice. """ @@ -414,7 +417,8 @@ def semantic_layer(*items: Any) -> SemanticLayer: Collecting the same measure twice -- the same file passed alongside its own directory, say -- is not an error; two different measures - sharing one name is. + sharing one name is. The R implementation is stricter here: it rejects + a repeated name even when it is the same measure twice. """ measures: dict[str, Measure] = {} source_text: dict[str, str] = {} @@ -481,22 +485,39 @@ def _from_path(path: Path) -> tuple[list[Measure], dict[str, str]]: f"directories of them." ) - # Not recursive, and __init__.py is skipped: a directory of measure files - # is a directory, not a package. - files = ( - sorted( + if path.is_dir(): + # Not recursive, and __init__.py is skipped: a directory of measure + # files is a directory, not a package. + files = sorted( entry for entry in path.iterdir() if entry.suffix == ".py" and entry.name != "__init__.py" ) - if path.is_dir() - else [path] - ) + # Once for the whole directory, before anything in it executes: the + # check scans every entry anyway, so repeating it per file is O(n^2) + # find_spec calls, and failing before the first load keeps a rejected + # directory from partially executing. + _check_directory_importable(path, requested=path) + directory_checked = True + else: + # The suffix is checked here, not left for the loader to discover: + # spec_from_file_location gives a .pyc or .so a real loader, so + # without this check such a file would execute as bytecode or native + # code instead of raising the error below. + if path.suffix != ".py": + raise ValueError( + f"Cannot read measures from {path}: not a Python file.\n" + f"Pass a .py file, a directory of them, or a module object." + ) + files = [path] + directory_checked = False measures: list[Measure] = [] sources: dict[str, str] = {} for file in files: - found, text = _from_module(_load_module_from_path(file)) + found, text = _from_module( + _load_module_from_path(file, directory_checked=directory_checked) + ) measures.extend(found) _merge_sources(sources, text) return measures, sources @@ -521,11 +542,14 @@ def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: """ measures: list[Measure] = [] sources: dict[str, str] = {} - for name, value in vars(module).items(): + for value in vars(module).values(): if inspect.isfunction(value): if value.__module__ != module.__name__: continue - sources[name] = _source_text(value) + # Keyed by the function's own name, not the binding: an alias + # (`also_known_as = measure_fn`) is one function and gets one + # entry, matching how directly-passed measures are keyed. + sources.setdefault(value.__name__, _source_text(value)) record = as_measure(value) else: # A measure can also sit at module level as a bare record. One @@ -568,7 +592,9 @@ def _from_module(module: ModuleType) -> tuple[list[Measure], dict[str, str]]: _load_mtimes: dict[str, int] = {} -def _load_module_from_path(path: Path) -> ModuleType: +def _load_module_from_path( + path: Path, *, directory_checked: bool = False +) -> ModuleType: resolved = path.resolve() stem = _UNSAFE_NAME_CHARS.sub("_", path.stem) # The digest, not a load counter: two semantic_layer() calls on the same @@ -606,7 +632,10 @@ def _load_module_from_path(path: Path) -> ModuleType: # _invalidate_bytecode_cache for why this step is required at all. _invalidate_bytecode_cache(path) - _check_directory_importable(path.parent, requested=path) + # Skipped when the caller already checked this directory: _from_path + # checks a directory once up front rather than once per file in it. + if not directory_checked: + _check_directory_importable(path.parent, requested=path) spec = importlib.util.spec_from_file_location(name, path) if spec is None or spec.loader is None: diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index 1b4acc2f..62b62c54 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -3,6 +3,7 @@ import enum import importlib import os +import py_compile import sys import threading from collections.abc import AsyncIterator @@ -18,6 +19,7 @@ INJECTED, Injected, Measure, + _from_module, _load_module_from_path, _load_mtimes, _split_parameters, @@ -955,6 +957,26 @@ def test_installed_but_unimported_module_is_a_construction_error( assert "already-importable" in message +def test_a_same_named_module_without_a_spec_is_a_construction_error( + tmp_path: Path, +) -> None: + # A module planted straight into sys.modules (a stub or test double, + # say) has no __spec__ for find_spec() to return, so the collision + # check falls back to looking in sys.modules itself. + (tmp_path / "planted.py").write_text( + "from commons._measures import measure\n\n\n" + "@measure(description='d')\n" + "def m() -> int:\n" + " return 1\n" + ) + sys.modules["planted"] = ModuleType("planted") + try: + with pytest.raises(ValueError, match="already imported from"): + semantic_layer(tmp_path / "planted.py") + finally: + sys.modules.pop("planted", None) + + def test_semantic_layer_reenters_during_a_measure_files_import() -> None: # A non-reentrant lock deadlocks here rather than raising, so this runs # on a daemon thread with a timeout: a regression fails the test instead @@ -990,6 +1012,17 @@ def test_semantic_layer_supports_membership_and_iteration() -> None: assert list(layer) == ["order_count"] +def test_semantic_layer_mappings_are_read_only() -> None: + # Only text and frozen records leave the layer; a plain dict here would + # let a caller mutate the layer after construction. + layer = semantic_layer(_count_measure()) + + with pytest.raises(TypeError): + layer.measures["other"] = _count_measure() # type: ignore[index] + with pytest.raises(TypeError): + layer.source_text["other"] = "def other(): ..." # type: ignore[index] + + def test_a_directory_and_a_file_inside_it_overlap_without_error() -> None: layer = semantic_layer( MEASURE_FILES / "sibling_imports", @@ -1005,6 +1038,13 @@ def test_a_module_level_alias_is_collected_once() -> None: assert list(layer.measures) == ["aliased_measure"] +def test_a_module_level_alias_shares_one_source_text_entry() -> None: + # Keyed by function name, not binding: the alias is the same function. + layer = semantic_layer(MEASURE_FILES / "aliased" / "aliased.py") + + assert set(layer.source_text) == {"aliased_measure"} + + def test_distinct_measures_from_two_files_sharing_a_name_are_an_error() -> None: with pytest.raises(ValueError, match="dup"): semantic_layer( @@ -1021,6 +1061,32 @@ def test_a_non_python_file_path_is_an_error(tmp_path: Path) -> None: semantic_layer(notes) +def test_a_compiled_bytecode_path_is_an_error(tmp_path: Path) -> None: + # spec_from_file_location gives a .pyc a real loader, so without an + # explicit suffix check this path would execute as bytecode. + source = tmp_path / "m.py" + source.write_text("VALUE = 1\n") + compiled = tmp_path / "m.pyc" + py_compile.compile(str(source), cfile=str(compiled)) + + with pytest.raises(ValueError, match="not a Python file"): + semantic_layer(compiled) + + +def test_a_non_python_file_is_rejected_before_the_directory_check( + tmp_path: Path, +) -> None: + # The requested file's error must win over a sibling's collision: the + # sibling is only checked because its directory would go on sys.path, + # which never happens for a file that is not Python at all. + (tmp_path / "json.py").write_text("VALUE = 1\n") + notes = tmp_path / "notes.txt" + notes.write_text("not python\n") + + with pytest.raises(ValueError, match="not a Python file"): + semantic_layer(notes) + + def test_a_directorys_init_py_is_never_imported() -> None: # has_init/__init__.py raises if it is ever executed. layer = semantic_layer(MEASURE_FILES / "has_init") @@ -1074,3 +1140,17 @@ def test_a_module_level_bare_record_is_harvested() -> None: assert list(layer.measures) == ["grand_total"] assert layer.measures["grand_total"].func() == 1 assert "def total" in layer.source_text["total"] + + +def test_an_imported_bare_record_is_not_harvested() -> None: + # A bare record re-exported by another module belongs to the module + # that defined its function, same as an imported function. + module = _load_module_from_path(MEASURE_FILES / "bare_record" / "total.py") + + reexporter = ModuleType("reexporter") + reexporter.grand_total = module.grand_total + + measures, sources = _from_module(reexporter) + + assert measures == [] + assert sources == {} From 0f3eb2bf871b37810deb8aee3b6ca79387845265 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Fri, 4 Sep 2026 10:25:57 -0600 Subject: [PATCH 18/19] test(py): write the planted reexporter module through its __dict__ pyrefly rejects an attribute assignment on a bare ModuleType; the test's point is the module's contents, not how they got there. --- pkg-py/tests/test_measures.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg-py/tests/test_measures.py b/pkg-py/tests/test_measures.py index 62b62c54..70f87273 100644 --- a/pkg-py/tests/test_measures.py +++ b/pkg-py/tests/test_measures.py @@ -1148,7 +1148,7 @@ def test_an_imported_bare_record_is_not_harvested() -> None: module = _load_module_from_path(MEASURE_FILES / "bare_record" / "total.py") reexporter = ModuleType("reexporter") - reexporter.grand_total = module.grand_total + reexporter.__dict__["grand_total"] = module.grand_total measures, sources = _from_module(reexporter) From 098b6a0715b7708df0aa5d5f99992a9f3d886caf Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Fri, 4 Sep 2026 10:29:08 -0600 Subject: [PATCH 19/19] docs: list the full pkg-py check gate in AGENTS.md The documented routine omitted pyrefly, so a type-check error reached CI that a local run would have caught. Match CI's steps and keep the warning about pyrefly's explicit src tests paths. --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 1e340d73..aee86b26 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,7 +2,7 @@ This repository is a monorepo holding two implementations of commons: the R pack If you worked in this repository before the two packages were split apart, `MIGRATING.md` covers what moved and what to change in your setup. -Work from the relevant package's directory, not the repository root: `pkg-r/` for R (`devtools::load_all()`, `R CMD check`) and `pkg-py/` for Python (`uv run pytest`, `uv run ruff check`). CI is scoped the same way. +Work from the relevant package's directory, not the repository root: `pkg-r/` for R (`devtools::load_all()`, `R CMD check`) and `pkg-py/` for Python (`uv run ruff check`, `uv run pyrefly check src tests`, `uv run pytest`). CI is scoped the same way. Run all of a package's checks before pushing; the pyrefly invocation needs its explicit `src tests` paths, because with none it consults the repo's git ignore files and a worktree checked out under an ignored directory silently type-checks nothing. Neither package has been widely adopted or publicly released; changes can be made without a deprecation cycle (or even reference to the way that it used to work).