From e5cb7ca72d3d6cb89ae290f1d2dc6511f4f52eef Mon Sep 17 00:00:00 2001 From: Ethan Date: Tue, 28 Jul 2026 22:24:45 +0800 Subject: [PATCH 1/3] fix(scripts): enforce relative-import scope in check_layer_imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLAUDE.md `## Import Policy` and CONTRIBUTING.md:59 both state "same package: relative imports; cross-package: absolute imports", but no enforcer covered the second half — a `from ..parent import X` passed every check. - Add `_check_relative_scope`: a relative import with level >= 2 escapes its own package, so it is a cross-package import written relatively. Flagged, with the resolved absolute module offered as the fix. - Narrow the private-access carve-out from `level > 0` to `level == 1`. Its comment claimed relative imports are "same-package by construction", which only holds at level 1; a `from .._x import _foo` slipped past both the private-name and protected-module rules. - Resolve level >= 2 relative imports to absolute before applying those two rules — `node.module` is the bare tail ("ir"), so every rule short-circuited on the `sieval.` prefix test. `sieval/` and `scripts/` are clean under the new check today; full preflight passes. Co-Authored-By: Claude Opus 5 (1M context) --- .pre-commit-config.yaml | 2 +- scripts/check_layer_imports.py | 83 +++++++- .../unit/scripts/test_check_layer_imports.py | 194 ++++++++++++++++++ 3 files changed, 274 insertions(+), 5 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 451317c8..b47a65b5 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -51,7 +51,7 @@ repos: - repo: local hooks: - id: layer-imports - name: check layer boundary + private-access imports + name: check layer boundary + private-access + relative-import scope entry: python scripts/check_layer_imports.py language: python types: [python] diff --git a/scripts/check_layer_imports.py b/scripts/check_layer_imports.py index 8dbc583c..b8d399ae 100644 --- a/scripts/check_layer_imports.py +++ b/scripts/check_layer_imports.py @@ -22,6 +22,18 @@ subtree may reach into them. Peer-subpackage access or out-of-subtree access is flagged. +3. **Relative-import scope** (the other half of CLAUDE.md `## Import Policy`: + "Same package: relative imports. Cross-package: absolute imports."): + + * ``from .sibling import X`` (level 1) is same-package — always fine. + * ``from ..parent import X`` (level >= 2) escapes the package and is + therefore a cross-package import written relatively. Flagged; use the + absolute ``from sieval.a.b import X`` form. + + This also closes a hole in check 2: its carve-out was written for + same-package relative imports but implemented as ``level > 0``, so a + ``from ..peer import _foo`` slipped past the private-name rule. + AI-Generated Code - Claude Opus 4.6 (Anthropic) """ @@ -120,6 +132,23 @@ def _file_package(path: Path) -> str: return ".".join(parts[idx:-1]) +def _resolve_relative(file_pkg: str, level: int, module: str) -> str | None: + """Resolve a relative import to its absolute dotted module. + + ``level`` follows :class:`ast.ImportFrom` semantics: 1 is the current + package, each extra level strips one trailing component. Returns ``None`` + when *file_pkg* is unknown or the level walks above the package root. + """ + if not file_pkg: + return None + parts = file_pkg.split(".") + strip = level - 1 + if strip >= len(parts): + return None + base = parts[: len(parts) - strip] if strip else parts + return ".".join([*base, module]) if module else ".".join(base) + + def _is_within_subtree(file_pkg: str, root: str) -> bool: """Return True if *file_pkg* equals *root* or descends from it.""" if not root: @@ -211,11 +240,23 @@ def _check_private_access(path: Path, tree: ast.AST) -> list[str]: for node in ast.walk(tree): if isinstance(node, ast.ImportFrom): - # Relative imports are same-package by construction; they are the - # carve-out that makes the `_base.py` sibling pattern legal. - if node.level > 0: + # Level-1 relative imports are same-package by construction; they + # are the carve-out that makes the `_base.py` sibling pattern legal. + # Level >= 2 escapes the package, so it is NOT covered by the + # carve-out — `_check_relative_scope` rejects it outright, and the + # private-name rule below must still see it if that check is ever + # relaxed. + if node.level == 1: continue - module = node.module or "" + if node.level >= 2: + # Resolve to absolute so the rules below actually apply. Without + # this, `node.module` is the bare tail ("ir") and every rule + # short-circuits on the `sieval.` prefix test. + module = ( + _resolve_relative(file_pkg, node.level, node.module or "") or "" + ) + else: + module = node.module or "" # Cover both `from sieval import _x` and `from sieval.pkg import …`. if module != "sieval" and not module.startswith("sieval."): continue @@ -251,6 +292,39 @@ def _check_private_access(path: Path, tree: ast.AST) -> list[str]: return errors +def _check_relative_scope(path: Path, tree: ast.AST) -> list[str]: + """Reject relative imports that escape their own package (level >= 2). + + CLAUDE.md `## Import Policy`: "Same package: relative imports. Cross-package: + absolute imports." A ``from ..parent import X`` is a cross-package import + written relatively, so it violates the second half of that rule. + + Scoped to the sieval package only: ``scripts/`` files are standalone modules, + not a package, so a relative import there fails at runtime and needs no + lint. (The pre-commit hook feeds both trees; this check narrows on purpose.) + """ + if _get_layer(path) is None: + return [] + if "tests" in path.parts: + return [] + + file_pkg = _file_package(path) + errors: list[str] = [] + + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.level >= 2: + written = "." * node.level + (node.module or "") + absolute = _resolve_relative(file_pkg, node.level, node.module or "") + fix = f"; use `from {absolute} import ...`" if absolute else "" + errors.append( + f"{path}:{node.lineno}: " + f"cross-package relative import {written!r} — relative imports " + f"are for the same package only, use absolute across packages" + f"{fix}" + ) + return errors + + def _check_file(path: Path) -> list[str]: try: tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) @@ -258,6 +332,7 @@ def _check_file(path: Path) -> list[str]: return [] errors = _check_layer_imports(path, tree) errors.extend(_check_private_access(path, tree)) + errors.extend(_check_relative_scope(path, tree)) return errors diff --git a/tests/unit/scripts/test_check_layer_imports.py b/tests/unit/scripts/test_check_layer_imports.py index 25e09834..7c343f9c 100644 --- a/tests/unit/scripts/test_check_layer_imports.py +++ b/tests/unit/scripts/test_check_layer_imports.py @@ -15,9 +15,11 @@ from check_layer_imports import ( # noqa: E402 # type: ignore[unresolved-import] # scripts/ added to sys.path at runtime _check_file, _check_private_access, + _check_relative_scope, _file_package, _get_layer, _is_within_subtree, + _resolve_relative, main, ) @@ -579,3 +581,195 @@ def test_both_rules_fire_on_same_import(self, tmp_path: Path): assert any( "import from private module 'sieval.tasks._hidden'" in e for e in errors ) + + +class TestResolveRelative: + """Relative -> absolute module resolution (ast.ImportFrom level semantics).""" + + def test_level_1_is_current_package(self): + assert ( + _resolve_relative("sieval.core.models", 1, "ir") == "sieval.core.models.ir" + ) + + def test_level_2_strips_one_component(self): + assert ( + _resolve_relative("sieval.core.models.transports", 2, "ir") + == "sieval.core.models.ir" + ) + + def test_level_3_strips_two_components(self): + assert ( + _resolve_relative("sieval.core.models.transports", 3, "utils") + == "sieval.core.utils" + ) + + def test_bare_relative_without_module(self): + # `from .. import x` -> the parent package itself + assert _resolve_relative("sieval.core.models.transports", 2, "") == ( + "sieval.core.models" + ) + + def test_walking_above_root_returns_none(self): + assert _resolve_relative("sieval.core", 3, "x") is None + + def test_unknown_package_returns_none(self): + assert _resolve_relative("", 2, "x") is None + + +class TestCheckRelativeScope: + """Rule 3: relative imports are same-package only (level >= 2 is a violation).""" + + def _write(self, tmp_path: Path, rel: str, src: str) -> Path: + f = tmp_path / rel + f.parent.mkdir(parents=True, exist_ok=True) + f.write_text(src) + return f + + def test_level_1_sibling_is_allowed(self, tmp_path: Path): + # `from ._base import X` is the legal same-package pattern. + f = self._write( + tmp_path, + "sieval/tasks/arc_easy_kshot_ppl.py", + "from ._arc import echoed_logprob\n", + ) + assert _check_relative_scope(f, __import__("ast").parse(f.read_text())) == [] + + def test_level_2_parent_import_is_error(self, tmp_path: Path): + f = self._write( + tmp_path, + "sieval/core/models/transports/openai_chat.py", + "from ..ir import Request\n", + ) + errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + assert len(errors) == 1 + assert "cross-package relative import '..ir'" in errors[0] + # The fix suggestion must name the resolved absolute module. + assert "from sieval.core.models.ir import" in errors[0] + + def test_level_3_is_error(self, tmp_path: Path): + f = self._write( + tmp_path, + "sieval/core/models/transports/sglang.py", + "from ...utils.concurrency import CompositeLimiter\n", + ) + errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + assert len(errors) == 1 + assert "from sieval.core.utils.concurrency import" in errors[0] + + def test_bare_parent_import_is_error(self, tmp_path: Path): + f = self._write( + tmp_path, + "sieval/core/models/transports/x.py", + "from .. import ir\n", + ) + errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + assert len(errors) == 1 + assert "'..'" in errors[0] + + def test_error_includes_lineno(self, tmp_path: Path): + f = self._write( + tmp_path, + "sieval/core/models/transports/x.py", + "import os\n\n\nfrom ..ir import Request\n", + ) + errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + assert ":4:" in errors[0] + + def test_multiple_violations_reported_separately(self, tmp_path: Path): + f = self._write( + tmp_path, + "sieval/core/models/transports/x.py", + "from ..ir import Request\nfrom ..capabilities import Capability\n", + ) + errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + assert len(errors) == 2 + + def test_tests_tree_is_exempt(self, tmp_path: Path): + f = self._write( + tmp_path, + "tests/unit/core/models/x.py", + "from ..conftest import helper\n", + ) + assert _check_relative_scope(f, __import__("ast").parse(f.read_text())) == [] + + def test_scripts_tree_is_out_of_scope(self, tmp_path: Path): + # scripts/ files are standalone modules, not a package. + f = self._write(tmp_path, "scripts/x.py", "from ..y import z\n") + assert _check_relative_scope(f, __import__("ast").parse(f.read_text())) == [] + + +class TestCheckFileRelativeScopeIntegration: + """Rule 3 wired into _check_file, and its interaction with rule 2.""" + + def _write(self, tmp_path: Path, rel: str, src: str) -> Path: + f = tmp_path / rel + f.parent.mkdir(parents=True, exist_ok=True) + f.write_text(src) + return f + + def test_level_1_relative_import_stays_clean(self, tmp_path: Path): + f = self._write( + tmp_path, + "sieval/tasks/arc_easy_kshot_ppl.py", + "from ._arc import echoed_logprob\n", + ) + assert _check_file(f) == [] + + def test_level_1_private_name_stays_clean(self, tmp_path: Path): + # CLAUDE.md: same-package siblings may reach into `._x`. + f = self._write( + tmp_path, + "sieval/tasks/foo.py", + "from ._arc import _helper\n", + ) + assert _check_file(f) == [] + + def test_parent_relative_import_is_reported(self, tmp_path: Path): + f = self._write( + tmp_path, + "sieval/core/models/transports/openai_chat.py", + "from ..capabilities import Capability\n", + ) + errors = _check_file(f) + assert any("cross-package relative import" in e for e in errors) + + def test_parent_relative_private_name_now_caught(self, tmp_path: Path): + # Regression: the private-access carve-out used to skip ALL relative + # imports (`level > 0`), so a cross-package `from .._x import _foo` + # slipped past both the private-name and protected-module rules. + f = self._write( + tmp_path, + "sieval/core/models/transports/openai_chat.py", + "from .._secret import _foo\n", + ) + errors = _check_file(f) + assert any("import of private name '_foo'" in e for e in errors) + assert any("cross-package relative import" in e for e in errors) + # The protected-MODULE rule correctly stays quiet here: `transports` is + # a descendant of `sieval.core.models`, and CLAUDE.md lets descendants + # reach into an ancestor's private module. + assert not any("import from private module" in e for e in errors) + + def test_parent_relative_out_of_subtree_module_now_caught(self, tmp_path: Path): + # Same carve-out regression, protected-MODULE half: a relative hop into + # a PEER subpackage's private module is out-of-subtree and must be + # flagged once the relative path is resolved to absolute. + f = self._write( + tmp_path, + "sieval/core/models/transports/openai_chat.py", + "from ...runners._internal import Helper\n", + ) + errors = _check_file(f) + assert any( + "import from private module 'sieval.core.runners._internal'" in e + for e in errors + ) + assert any("cross-package relative import" in e for e in errors) + + def test_parent_relative_above_root_does_not_crash(self, tmp_path: Path): + # `_resolve_relative` returns None; the check must still report the + # violation, just without a fix suggestion. + f = self._write(tmp_path, "sieval/core/x.py", "from ... import y\n") + errors = _check_file(f) + assert any("cross-package relative import" in e for e in errors) + assert all("use `from" not in e for e in errors) From 9914f67e6706ec3e7ade26dccb8b91fb4180ff96 Mon Sep 17 00:00:00 2001 From: Ethan Date: Wed, 29 Jul 2026 00:40:55 +0800 Subject: [PATCH 2/3] fix(scripts): catch cross-layer imports written relatively or via alias MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_check_layer_imports` matched on `node.module`, which for a relative import is only the bare tail ("tasks" for `from ...tasks import x`). Such an import short-circuited on the `sieval.` prefix test and was reported solely as an import-style error by `_check_relative_scope` — whose suggested fix is itself a layer violation. `from sieval import tasks` went unreported entirely: it names the layer as an imported alias rather than in the module path, so the `len(parts) >= 2` test never fired. The two holes chained — `_check_relative_scope` offers `from sieval import ...` as the fix for `from ... import tasks`, steering authors toward the one shape the layer check could not see. - extract `_absolute_module` as the single relative->absolute normalization that both `_check_layer_imports` and `_check_private_access` read through, replacing the latter's inline branch so the contract lives in one place - match the imported-alias shape when the resolved module is bare `sieval` - docstring: stale "Two categories of check", `_check_layer_imports`'s "(existing behavior)", and both closed holes - tests: hoist `import ast` to module level (10 local/inline uses) Co-Authored-By: Claude Opus 5 (1M context) --- scripts/check_layer_imports.py | 71 ++++++++--- .../unit/scripts/test_check_layer_imports.py | 117 +++++++++++++++--- 2 files changed, 154 insertions(+), 34 deletions(-) diff --git a/scripts/check_layer_imports.py b/scripts/check_layer_imports.py index b8d399ae..1bc5ad15 100644 --- a/scripts/check_layer_imports.py +++ b/scripts/check_layer_imports.py @@ -1,7 +1,7 @@ """ Pre-commit hook: enforce sieval import policy. -Two categories of check: +Three categories of check: 1. **Layer boundary imports** — each layer has a hard-coded set of sibling layers it must not import from. Current map: @@ -30,9 +30,16 @@ therefore a cross-package import written relatively. Flagged; use the absolute ``from sieval.a.b import X`` form. - This also closes a hole in check 2: its carve-out was written for - same-package relative imports but implemented as ``level > 0``, so a - ``from ..peer import _foo`` slipped past the private-name rule. + Checks 1 and 2 resolve relative imports to absolute (``_absolute_module``) + before their own rules run, so a violation written relatively is diagnosed + as what it is rather than only as a style error. Two holes this closed: + + * Check 2's carve-out was written for same-package relative imports but + implemented as ``level > 0``, so ``from ..peer import _foo`` slipped past + the private-name rule. + * Check 1 matched on ``node.module`` alone, so ``from ...tasks import x`` in + ``core/`` went unreported — as did the absolute ``from sieval import + tasks``, which names the layer as an alias rather than in the module path. AI-Generated Code - Claude Opus 4.6 (Anthropic) """ @@ -149,6 +156,20 @@ def _resolve_relative(file_pkg: str, level: int, module: str) -> str | None: return ".".join([*base, module]) if module else ".".join(base) +def _absolute_module(node: ast.ImportFrom, file_pkg: str) -> str: + """Return the absolute dotted module *node* imports from, or ``""``. + + Absolute imports (level 0) are returned as written; relative ones are + resolved against *file_pkg* so every rule sees the same absolute form. + Without this, ``node.module`` is only the bare tail (``"tasks"`` for + ``from ...tasks import x``) and each rule short-circuits on its ``sieval.`` + prefix test. Empty when the level walks above the package root. + """ + if node.level == 0: + return node.module or "" + return _resolve_relative(file_pkg, node.level, node.module or "") or "" + + def _is_within_subtree(file_pkg: str, root: str) -> bool: """Return True if *file_pkg* equals *root* or descends from it.""" if not root: @@ -190,12 +211,19 @@ def _subtree_violation( def _check_layer_imports(path: Path, tree: ast.AST) -> list[str]: - """Layer-boundary check (existing behavior).""" - forbidden = FORBIDDEN.get(_get_layer(path) or "") + """Layer-boundary check: flag imports of a layer this file must not reach. + + Matches all three shapes a forbidden layer can be named in: the module path + (``import sieval.tasks`` / ``from sieval.tasks import X``, relative form + included via ``_absolute_module``) and the imported alias + (``from sieval import tasks``). + """ + layer = _get_layer(path) + forbidden = FORBIDDEN.get(layer or "") if not forbidden: return [] + file_pkg = _file_package(path) errors: list[str] = [] - layer = _get_layer(path) for node in ast.walk(tree): if isinstance(node, ast.Import): for alias in node.names: @@ -206,14 +234,27 @@ def _check_layer_imports(path: Path, tree: ast.AST) -> list[str]: f"{layer}/ must not import {parts[1]}/ " f"({alias.name})" ) - elif isinstance(node, ast.ImportFrom) and node.module: - parts = node.module.split(".") + elif isinstance(node, ast.ImportFrom): + module = _absolute_module(node, file_pkg) + parts = module.split(".") if len(parts) >= 2 and parts[0] == "sieval" and parts[1] in forbidden: errors.append( f"{path}:{node.lineno}: " f"{layer}/ must not import {parts[1]}/ " - f"({node.module})" + f"({module})" ) + elif module == "sieval": + # `from sieval import tasks` names the layer as an imported + # alias, not in the module path — same violation, different + # shape. Reachable relatively too (`from ... import tasks`), + # which is what `_check_relative_scope` suggests as the fix. + for alias in node.names: + if alias.name in forbidden: + errors.append( + f"{path}:{node.lineno}: " + f"{layer}/ must not import {alias.name}/ " + f"(sieval.{alias.name})" + ) return errors @@ -248,15 +289,7 @@ def _check_private_access(path: Path, tree: ast.AST) -> list[str]: # relaxed. if node.level == 1: continue - if node.level >= 2: - # Resolve to absolute so the rules below actually apply. Without - # this, `node.module` is the bare tail ("ir") and every rule - # short-circuits on the `sieval.` prefix test. - module = ( - _resolve_relative(file_pkg, node.level, node.module or "") or "" - ) - else: - module = node.module or "" + module = _absolute_module(node, file_pkg) # Cover both `from sieval import _x` and `from sieval.pkg import …`. if module != "sieval" and not module.startswith("sieval."): continue diff --git a/tests/unit/scripts/test_check_layer_imports.py b/tests/unit/scripts/test_check_layer_imports.py index 7c343f9c..05271e89 100644 --- a/tests/unit/scripts/test_check_layer_imports.py +++ b/tests/unit/scripts/test_check_layer_imports.py @@ -1,9 +1,11 @@ """ -Tests for scripts/check_layer_imports.py — layer boundary enforcement. +Tests for scripts/check_layer_imports.py — layer boundary, private-access and +relative-import-scope enforcement. AI-Generated Code - Claude Opus 4.6 (Anthropic) """ +import ast import sys from pathlib import Path @@ -13,6 +15,7 @@ sys.path.insert(0, _SCRIPTS_DIR) from check_layer_imports import ( # noqa: E402 # type: ignore[unresolved-import] # scripts/ added to sys.path at runtime + _absolute_module, _check_file, _check_private_access, _check_relative_scope, @@ -123,6 +126,55 @@ def test_import_from_detected(self, tmp_path: Path): assert len(errors) == 1 assert "core/ must not import tasks/" in errors[0] + def test_relative_cross_layer_import_reports_the_layer_violation( + self, tmp_path: Path + ): + # Regression: the layer check matched on `node.module`, which for a + # relative import is the bare tail ("tasks") and short-circuits on the + # `sieval.` prefix test. The violation was reported only as an import- + # style error, whose suggested fix is itself a layer violation. + f = tmp_path / "sieval" / "core" / "models" / "bad.py" + f.parent.mkdir(parents=True) + f.write_text("from ...tasks import registry\n") + errors = _check_file(f) + assert any("core/ must not import tasks/ (sieval.tasks)" in e for e in errors) + assert any("cross-package relative import" in e for e in errors) + + def test_from_sieval_import_layer_is_error(self, tmp_path: Path): + # `from sieval import tasks` names the layer as an alias, not in the + # module path — previously unreported entirely. + f = tmp_path / "sieval" / "core" / "bad.py" + f.parent.mkdir(parents=True) + f.write_text("from sieval import tasks\n") + errors = _check_file(f) + assert len(errors) == 1 + assert "core/ must not import tasks/ (sieval.tasks)" in errors[0] + + def test_bare_relative_import_of_layer_is_error(self, tmp_path: Path): + # `from ... import tasks` resolves to module "sieval" + alias "tasks" — + # exactly the shape `_check_relative_scope` suggests as the fix. + f = tmp_path / "sieval" / "core" / "models" / "bad.py" + f.parent.mkdir(parents=True) + f.write_text("from ... import tasks\n") + errors = _check_file(f) + assert any("core/ must not import tasks/ (sieval.tasks)" in e for e in errors) + + def test_from_sieval_import_public_name_is_allowed(self, tmp_path: Path): + # `from sieval import __version__` is a real pattern in core/ — the + # alias branch must not fire on names that are not layers. + f = tmp_path / "sieval" / "core" / "ok.py" + f.parent.mkdir(parents=True) + f.write_text("from sieval import __version__\n") + assert _check_file(f) == [] + + def test_same_package_relative_import_is_allowed(self, tmp_path: Path): + # Level 1 is same-package, therefore same-layer: it can never cross a + # layer boundary and must stay clean. + f = tmp_path / "sieval" / "core" / "ok.py" + f.parent.mkdir(parents=True) + f.write_text("from .sibling import helper\n") + assert _check_file(f) == [] + def test_syntax_error_returns_empty(self, tmp_path: Path): f = tmp_path / "sieval" / "core" / "broken.py" f.parent.mkdir(parents=True) @@ -440,11 +492,9 @@ def test_test_file_exempt(self, tmp_path: Path): "from sieval.core.datasets.meta import _INTERNAL_STATE\n" "from sieval.tasks._parse_utils import extract_boxed\n", ) - import ast as _ast - assert _get_layer(f) is None assert "tests" in f.parts - assert _check_private_access(f, _ast.parse(f.read_text())) == [] + assert _check_private_access(f, ast.parse(f.read_text())) == [] def test_nested_sieval_layer_tests_still_exempt(self, tmp_path: Path): # Edge case: a test file nested inside a sieval subpackage @@ -456,11 +506,9 @@ def test_nested_sieval_layer_tests_still_exempt(self, tmp_path: Path): "sieval/tasks/tests/test_x.py", "from sieval.core.datasets.meta import _INTERNAL_STATE\n", ) - import ast as _ast - assert _get_layer(f) == "tasks" assert "tests" in f.parts - assert _check_private_access(f, _ast.parse(f.read_text())) == [] + assert _check_private_access(f, ast.parse(f.read_text())) == [] # ── scripts/ enforcement ── @@ -616,6 +664,45 @@ def test_unknown_package_returns_none(self): assert _resolve_relative("", 2, "x") is None +class TestAbsoluteModule: + """The shared relative->absolute normalization every rule reads through.""" + + def _node(self, src: str) -> ast.ImportFrom: + node = ast.parse(src).body[0] + assert isinstance(node, ast.ImportFrom) + return node + + def test_absolute_import_passes_through(self): + node = self._node("from sieval.core.models import ir\n") + assert _absolute_module(node, "sieval.tasks") == "sieval.core.models" + + def test_absolute_import_without_module_is_empty(self): + # `from . import x` at level 0 is not expressible; guard the None branch. + node = self._node("from sieval import x\n") + node.module = None + assert _absolute_module(node, "sieval.tasks") == "" + + def test_level_1_resolves_to_own_package(self): + node = self._node("from .ir import Request\n") + assert _absolute_module(node, "sieval.core.models") == "sieval.core.models.ir" + + def test_level_2_resolves_to_parent(self): + node = self._node("from ..ir import Request\n") + assert ( + _absolute_module(node, "sieval.core.models.transports") + == "sieval.core.models.ir" + ) + + def test_bare_relative_resolves_to_the_package_itself(self): + node = self._node("from ... import tasks\n") + assert _absolute_module(node, "sieval.core.models") == "sieval" + + def test_above_root_is_empty_not_none(self): + # Callers do prefix tests on the result, so it must be a str. + node = self._node("from ... import y\n") + assert _absolute_module(node, "sieval.core") == "" + + class TestCheckRelativeScope: """Rule 3: relative imports are same-package only (level >= 2 is a violation).""" @@ -632,7 +719,7 @@ def test_level_1_sibling_is_allowed(self, tmp_path: Path): "sieval/tasks/arc_easy_kshot_ppl.py", "from ._arc import echoed_logprob\n", ) - assert _check_relative_scope(f, __import__("ast").parse(f.read_text())) == [] + assert _check_relative_scope(f, ast.parse(f.read_text())) == [] def test_level_2_parent_import_is_error(self, tmp_path: Path): f = self._write( @@ -640,7 +727,7 @@ def test_level_2_parent_import_is_error(self, tmp_path: Path): "sieval/core/models/transports/openai_chat.py", "from ..ir import Request\n", ) - errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + errors = _check_relative_scope(f, ast.parse(f.read_text())) assert len(errors) == 1 assert "cross-package relative import '..ir'" in errors[0] # The fix suggestion must name the resolved absolute module. @@ -652,7 +739,7 @@ def test_level_3_is_error(self, tmp_path: Path): "sieval/core/models/transports/sglang.py", "from ...utils.concurrency import CompositeLimiter\n", ) - errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + errors = _check_relative_scope(f, ast.parse(f.read_text())) assert len(errors) == 1 assert "from sieval.core.utils.concurrency import" in errors[0] @@ -662,7 +749,7 @@ def test_bare_parent_import_is_error(self, tmp_path: Path): "sieval/core/models/transports/x.py", "from .. import ir\n", ) - errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + errors = _check_relative_scope(f, ast.parse(f.read_text())) assert len(errors) == 1 assert "'..'" in errors[0] @@ -672,7 +759,7 @@ def test_error_includes_lineno(self, tmp_path: Path): "sieval/core/models/transports/x.py", "import os\n\n\nfrom ..ir import Request\n", ) - errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + errors = _check_relative_scope(f, ast.parse(f.read_text())) assert ":4:" in errors[0] def test_multiple_violations_reported_separately(self, tmp_path: Path): @@ -681,7 +768,7 @@ def test_multiple_violations_reported_separately(self, tmp_path: Path): "sieval/core/models/transports/x.py", "from ..ir import Request\nfrom ..capabilities import Capability\n", ) - errors = _check_relative_scope(f, __import__("ast").parse(f.read_text())) + errors = _check_relative_scope(f, ast.parse(f.read_text())) assert len(errors) == 2 def test_tests_tree_is_exempt(self, tmp_path: Path): @@ -690,12 +777,12 @@ def test_tests_tree_is_exempt(self, tmp_path: Path): "tests/unit/core/models/x.py", "from ..conftest import helper\n", ) - assert _check_relative_scope(f, __import__("ast").parse(f.read_text())) == [] + assert _check_relative_scope(f, ast.parse(f.read_text())) == [] def test_scripts_tree_is_out_of_scope(self, tmp_path: Path): # scripts/ files are standalone modules, not a package. f = self._write(tmp_path, "scripts/x.py", "from ..y import z\n") - assert _check_relative_scope(f, __import__("ast").parse(f.read_text())) == [] + assert _check_relative_scope(f, ast.parse(f.read_text())) == [] class TestCheckFileRelativeScopeIntegration: From cd45b16b0f41675ee7a7bec4956861a6d38cc12f Mon Sep 17 00:00:00 2001 From: Ethan Date: Wed, 29 Jul 2026 01:25:22 +0800 Subject: [PATCH 3/3] fix(scripts): narrow private-access carve-out to same-package imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Narrowing the carve-out from `level > 0` to `level == 1` was still wider than "same-package": a *dotted* level-1 module walks DOWN into a child subpackage, so `from .sub._hidden import X` resolved to `sieval.tasks.sub._hidden` and escaped the protected-module rule — while the identical absolute spelling was flagged. Same semantic import, two verdicts depending only on how it was written. The predicate is now `level == 1 and "." not in (node.module or "")`. No-op on the current tree (zero level-1 dotted relative imports under `sieval/` + `scripts/`), so this closes a latent hole rather than changing behavior. Residual limit, recorded in a comment and a test: `from .sub import _priv` is cross-package when `sub` is a package, but is syntactically identical to a sibling module (`from .mod import _priv`). Separating them needs a filesystem lookup, which would make the verdict depend on checkout completeness. Left exempt on purpose — the dotted form is where a private module segment can actually appear mid-path. Docs touched in the same pass: * `_check_relative_scope` docstring now states that level-1 dotted descent is knowingly permitted. Rule 3 is the style half only; checks 1 and 2 resolve through it either way, so layer boundaries and private-module protection are unaffected. * Module docstring: hoisted the "holes this closed" block out from under heading 3, which described fixes to checks 1 and 2. * `check_preflight.check_imports` no longer claims parity with the pre-commit file set. It names the `sieval/community/` divergence the global `exclude:` creates, why it is inert today, and why the fix is a design call. Behavior unchanged. Tests: 97 -> 102. The spelling-equivalence test kills reverting the predicate to `level == 1`, widening to `level > 0`, and `and` -> `or`. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/check_layer_imports.py | 57 +++++++++----- scripts/check_preflight.py | 12 ++- .../unit/scripts/test_check_layer_imports.py | 76 +++++++++++++++++++ 3 files changed, 127 insertions(+), 18 deletions(-) diff --git a/scripts/check_layer_imports.py b/scripts/check_layer_imports.py index 1bc5ad15..2d146fb5 100644 --- a/scripts/check_layer_imports.py +++ b/scripts/check_layer_imports.py @@ -30,16 +30,19 @@ therefore a cross-package import written relatively. Flagged; use the absolute ``from sieval.a.b import X`` form. - Checks 1 and 2 resolve relative imports to absolute (``_absolute_module``) - before their own rules run, so a violation written relatively is diagnosed - as what it is rather than only as a style error. Two holes this closed: - - * Check 2's carve-out was written for same-package relative imports but - implemented as ``level > 0``, so ``from ..peer import _foo`` slipped past - the private-name rule. - * Check 1 matched on ``node.module`` alone, so ``from ...tasks import x`` in - ``core/`` went unreported — as did the absolute ``from sieval import - tasks``, which names the layer as an alias rather than in the module path. +**Relative imports are resolved to absolute (``_absolute_module``) before any +rule runs**, so a violation written relatively is diagnosed as what it is +rather than only as an import-style error. Holes this closed: + +* Check 2's carve-out was written for same-package relative imports but + implemented as ``level > 0``, so ``from ..peer import _foo`` slipped past the + private-name rule. Narrowing it to ``level == 1`` alone was still too wide — + a *dotted* level-1 module walks DOWN into a child subpackage, so + ``from .sub._hidden import X`` escaped as well. ``_check_private_access`` + documents the residual limit. +* Check 1 matched on ``node.module`` alone, so ``from ...tasks import x`` in + ``core/`` went unreported — as did the absolute ``from sieval import + tasks``, which names the layer as an alias rather than in the module path. AI-Generated Code - Claude Opus 4.6 (Anthropic) """ @@ -281,13 +284,26 @@ def _check_private_access(path: Path, tree: ast.AST) -> list[str]: for node in ast.walk(tree): if isinstance(node, ast.ImportFrom): - # Level-1 relative imports are same-package by construction; they - # are the carve-out that makes the `_base.py` sibling pattern legal. - # Level >= 2 escapes the package, so it is NOT covered by the - # carve-out — `_check_relative_scope` rejects it outright, and the - # private-name rule below must still see it if that check is ever - # relaxed. - if node.level == 1: + # Carve-out: a level-1 relative import with an UNDOTTED module is + # same-package, which is what makes the `_base.py` sibling pattern + # legal. Both other shapes escape the package and must still reach + # the rules below: + # * level >= 2 walks UP out of the package (`from .._x import _y`); + # `_check_relative_scope` also rejects it outright, but the + # private rules must see it if that check is ever relaxed. + # * a dotted level-1 module walks DOWN into a child subpackage — + # `from .sub._hidden import X` resolves to + # `sieval.pkg.sub._hidden`, whose owning subtree is + # `sieval.pkg.sub`. The importer at `sieval.pkg` is an ancestor, + # not a descendant, so that access is out-of-subtree. + # Residual limit: an undotted level-1 module naming a *subpackage* + # (`from .sub import _priv`) is cross-package too, but is + # syntactically identical to a sibling *module* (`from .mod import + # _priv`) — telling them apart needs a filesystem lookup, which + # would make the verdict depend on checkout completeness. Left + # exempt on purpose; the dotted form above is where a private + # module segment can actually appear mid-path. + if node.level == 1 and "." not in (node.module or ""): continue module = _absolute_module(node, file_pkg) # Cover both `from sieval import _x` and `from sieval.pkg import …`. @@ -332,6 +348,13 @@ def _check_relative_scope(path: Path, tree: ast.AST) -> list[str]: absolute imports." A ``from ..parent import X`` is a cross-package import written relatively, so it violates the second half of that rule. + Deliberately *not* flagged: a dotted level-1 module (``from .sub.mod import + X``) also crosses into a child package, but reads as a local descent and is + idiomatic enough that banning it outright buys little. It is still resolved + to absolute for checks 1 and 2, so the rules that matter — layer boundaries + and private-module protection — see through it either way. This check is the + style half only. + Scoped to the sieval package only: ``scripts/`` files are standalone modules, not a package, so a relative import there fails at runtime and needs no lint. (The pre-commit hook feeds both trees; this check narrows on purpose.) diff --git a/scripts/check_preflight.py b/scripts/check_preflight.py index a94a210e..582a9e9f 100644 --- a/scripts/check_preflight.py +++ b/scripts/check_preflight.py @@ -1048,11 +1048,21 @@ def check_imports(self) -> list[CheckResult]: if not script.exists(): return [CheckResult("FAIL", "check_imports", f"script not found: {script}")] - # Must match the pre-commit hook's `files:` filter in + # Matches the pre-commit hook's `files:` filter in # `.pre-commit-config.yaml` (^(sieval|scripts)/). Narrowing here to # `sieval/` only would leave the script's `in_scripts` branch untested # by preflight while still running in pre-commit — two enforcement # surfaces silently diverging. + # + # KNOWN divergence, not parity: pre-commit additionally applies the + # global `exclude: ^(sieval/community/|vendor/)`, so it skips + # `sieval/community/` while this wrapper checks it. Inert today (every + # relative import under `community/` is a bare level-1 `from . import + # x`), but a future vendored drop using `from ..x import y` would pass + # pre-commit and fail preflight, and the only offered fix would be to + # edit code kept byte-identical to upstream. Fixing it is a design call + # — hoisting the exemption into `_check_file` would also drop the + # private-access check's coverage of `community/`. enforced_py = [ f for f in self._git_tracked_files(".py") diff --git a/tests/unit/scripts/test_check_layer_imports.py b/tests/unit/scripts/test_check_layer_imports.py index 05271e89..eb38dca3 100644 --- a/tests/unit/scripts/test_check_layer_imports.py +++ b/tests/unit/scripts/test_check_layer_imports.py @@ -721,6 +721,17 @@ def test_level_1_sibling_is_allowed(self, tmp_path: Path): ) assert _check_relative_scope(f, ast.parse(f.read_text())) == [] + def test_level_1_dotted_descent_is_allowed(self, tmp_path: Path): + # Rule 3 is the style half only: `from .sub.mod import X` crosses into a + # child package but reads as a local descent and is not flagged here. + # Checks 1 and 2 still see through it (see the integration tests). + f = self._write( + tmp_path, + "sieval/tasks/foo.py", + "from .sub.mod import helper\n", + ) + assert _check_relative_scope(f, ast.parse(f.read_text())) == [] + def test_level_2_parent_import_is_error(self, tmp_path: Path): f = self._write( tmp_path, @@ -860,3 +871,68 @@ def test_parent_relative_above_root_does_not_crash(self, tmp_path: Path): errors = _check_file(f) assert any("cross-package relative import" in e for e in errors) assert all("use `from" not in e for e in errors) + + def test_level_1_dotted_into_child_private_module_is_caught(self, tmp_path: Path): + # Regression: narrowing the private-access carve-out to `level == 1` was + # still too wide. A DOTTED level-1 module walks down into a child + # subpackage, so `.sub._hidden` resolves to `sieval.tasks.sub._hidden` + # — owned by subtree `sieval.tasks.sub`, which the importer at + # `sieval.tasks` is an ancestor of, not a descendant. Out-of-subtree. + f = self._write( + tmp_path, + "sieval/tasks/foo.py", + "from .sub._hidden import X\n", + ) + errors = _check_file(f) + assert any( + "import from private module 'sieval.tasks.sub._hidden'" in e for e in errors + ) + # Rule 3 is the style half and deliberately stays quiet on level-1. + assert not any("cross-package relative import" in e for e in errors) + + def test_dotted_level_1_matches_its_absolute_spelling(self, tmp_path: Path): + # The point of resolving before the rules run: the same semantic import + # must get the same verdict whichever way it is spelled. This equivalence + # is what the carve-out bug broke. + rel = self._write( + tmp_path / "rel", + "sieval/tasks/foo.py", + "from .sub._hidden import X\n", + ) + absolute = self._write( + tmp_path / "abs", + "sieval/tasks/foo.py", + "from sieval.tasks.sub._hidden import X\n", + ) + + def _strip(errors: list[str], base: Path) -> list[str]: + return sorted(e.replace(str(base), "") for e in errors) + + assert _strip(_check_file(rel), tmp_path / "rel") == _strip( + _check_file(absolute), tmp_path / "abs" + ) + + def test_level_1_dotted_public_path_stays_clean(self, tmp_path: Path): + # No private segment, no private name — the tightened carve-out must not + # start flagging ordinary descents into a subpackage. + f = self._write( + tmp_path, + "sieval/tasks/foo.py", + "from .sub.mod import helper\n", + ) + assert _check_file(f) == [] + + def test_level_1_undotted_subpackage_private_name_stays_exempt( + self, tmp_path: Path + ): + # Documents the residual limit: `from .sub import _priv` is cross-package + # when `sub` is a package, but is syntactically identical to a sibling + # module (`from .mod import _priv`). Distinguishing them needs a + # filesystem lookup, which would make the verdict depend on checkout + # completeness — left exempt on purpose. + f = self._write( + tmp_path, + "sieval/tasks/foo.py", + "from .sub import _priv\n", + ) + assert _check_file(f) == []