fix: the roster guard's import walk resolved package __init__ imports wrongly (387 C5)
CI / lint (push) Successful in 2s
Build images / sign-extension (push) Successful in 3s
CI / extension-version (push) Successful in 1s
Build images / build-agent (push) Successful in 6s
Build images / build-ml (push) Successful in 6s
CI / frontend-build (push) Successful in 30s
CI / backend-lint-and-test (push) Successful in 32s
Build images / build-web (push) Successful in 7s
Build images / smoke-web (push) Skipped
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m37s
CI / lint (push) Successful in 2s
Build images / sign-extension (push) Successful in 3s
CI / extension-version (push) Successful in 1s
Build images / build-agent (push) Successful in 6s
Build images / build-ml (push) Successful in 6s
CI / frontend-build (push) Successful in 30s
CI / backend-lint-and-test (push) Successful in 32s
Build images / build-web (push) Successful in 7s
Build images / smoke-web (push) Skipped
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m37s
Two of C5's three structural tests errored in CI with ValueError: PosixPath('.') has an empty name. The walk special-cased a package's __init__.py, dropping the __init__ component before computing what `from .` refers to — which made `api/__init__.py`'s `from . import health` resolve to the app root instead of to `api`, and `celery_app.py`'s `from . import celery_signals` resolve to the empty string, which is what actually crashed.
The special case was never needed: `parts[:-1]` already gives the CONTAINING package for both forms, because `services/foo.py` drops `foo` to leave `services` and `api/__init__.py` drops `__init__` to leave `api` — exactly what `from .` means inside each. The level slice is now clamped at 0 as well; an import climbing past backend/app left the tree, and the unclamped negative index wrapped and resolved to the wrong module rather than to nothing.
A module directly under backend/app doing `from . import x` still yields no package prefix, and there the alias alone IS the dotted name - handled explicitly rather than by falling through into a Path built from an empty string.
The two positive controls earned their place immediately: they are what failed. Without them the walk would have resolved almost nothing and test_no_fetch_path_can_read_the_roster would have passed on a broken walker, reading as coverage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SHQB1YukL3VyvMK8rcbmV9
This commit is contained in:
+23
-12
@@ -282,24 +282,32 @@ def _first_party_imports(path: Path) -> set[str]:
|
|||||||
"""Every `backend.app.*` module this file imports, as a dotted path
|
"""Every `backend.app.*` module this file imports, as a dotted path
|
||||||
relative to `backend/app` — absolute and relative forms both."""
|
relative to `backend/app` — absolute and relative forms both."""
|
||||||
tree = ast.parse(path.read_text())
|
tree = ast.parse(path.read_text())
|
||||||
here = path.relative_to(_APP).with_suffix("").parts
|
# The dotted parts of the package CONTAINING this module. `parts[:-1]` is
|
||||||
if here and here[-1] == "__init__":
|
# right for both forms without a special case: `services/foo.py` drops
|
||||||
# A package's `__init__` IS the package, so `from .x import y` inside it
|
# `foo` to leave `services`, and `api/__init__.py` drops `__init__` to
|
||||||
# resolves one level shallower than the file path suggests. Getting this
|
# leave `api` — which is exactly what `from .` means inside each.
|
||||||
# wrong silently under-resolves every relative import in every package
|
pkg = path.relative_to(_APP).with_suffix("").parts[:-1]
|
||||||
# and would make the guard below unable to fail.
|
|
||||||
here = here[:-1]
|
|
||||||
out: set[str] = set()
|
out: set[str] = set()
|
||||||
for node in ast.walk(tree):
|
for node in ast.walk(tree):
|
||||||
if isinstance(node, ast.ImportFrom):
|
if isinstance(node, ast.ImportFrom):
|
||||||
if node.level:
|
if node.level:
|
||||||
# `from ..models import X` -> walk up from this module's package
|
# `from ..models import X` -> walk up from the containing
|
||||||
base = list(here[: len(here) - node.level])
|
# package. Clamped at 0: a level that climbs past `backend.app`
|
||||||
mod = base + (node.module.split(".") if node.module else [])
|
# leaves this tree, and an unclamped negative index would wrap
|
||||||
|
# and silently resolve to the wrong module.
|
||||||
|
up = max(len(pkg) - (node.level - 1), 0)
|
||||||
|
mod = list(pkg[:up]) + (node.module.split(".") if node.module else [])
|
||||||
elif node.module and node.module.startswith("backend.app."):
|
elif node.module and node.module.startswith("backend.app."):
|
||||||
mod = node.module[len("backend.app."):].split(".")
|
mod = node.module[len("backend.app."):].split(".")
|
||||||
else:
|
else:
|
||||||
continue
|
continue
|
||||||
|
if not mod:
|
||||||
|
# `from . import x` in a module sitting directly under
|
||||||
|
# `backend/app` (celery_app.py does this): the package is the
|
||||||
|
# app root, so the alias alone is the module's dotted name.
|
||||||
|
for alias in node.names:
|
||||||
|
out.add(alias.name)
|
||||||
|
continue
|
||||||
out.add(".".join(mod))
|
out.add(".".join(mod))
|
||||||
# `from .membership_roster import x` and
|
# `from .membership_roster import x` and
|
||||||
# `from . import membership_roster` must both resolve to the module.
|
# `from . import membership_roster` must both resolve to the module.
|
||||||
@@ -320,8 +328,11 @@ def _reachable_from(roots: list[str]) -> set[str]:
|
|||||||
if mod in seen:
|
if mod in seen:
|
||||||
continue
|
continue
|
||||||
seen.add(mod)
|
seen.add(mod)
|
||||||
for candidate in (_APP / Path(*mod.split(".")) / "__init__.py",
|
parts = [p for p in mod.split(".") if p]
|
||||||
_APP / Path(*mod.split(".")).with_suffix(".py")):
|
if not parts:
|
||||||
|
continue
|
||||||
|
for candidate in (_APP.joinpath(*parts) / "__init__.py",
|
||||||
|
_APP.joinpath(*parts).with_suffix(".py")):
|
||||||
if candidate.is_file():
|
if candidate.is_file():
|
||||||
queue.extend(_first_party_imports(candidate) - seen)
|
queue.extend(_first_party_imports(candidate) - seen)
|
||||||
break
|
break
|
||||||
|
|||||||
Reference in New Issue
Block a user