chore(devex): ratchet against eager __init__.py re-exports via semgrep - #71445
Conversation
Eager top-level re-exports in __init__.py pull a package's whole submodule subtree onto the import path, so every entrypoint (web, celery, temporal, CI, shell) pays the cost, and cross-package re-exports are a common source of import cycles. House style is deep imports from the defining module; convenience aliases should be lazy (PEP 562 __getattr__) or TYPE_CHECKING-only. New ERROR rule in the semgrep-devex set, run purely as a ratchet: it blocks new packages from adopting the pattern while grandfathering every existing occurrence under paths.exclude (60 files). No burn-down is implied or required — the list just freezes today's set so master stays green and the pattern stops spreading. The pattern is anchored at column 0 ((?m)^from\s+\.), so only eager top-level re-exports match; indented ones (TYPE_CHECKING, try/except ImportError, function-local) are already lazy and skipped with no allowlist to maintain. The "deemed alright" boundaries are exempt by path rather than listed individually, so they stay self-maintaining: Django model aggregators (**/models/), facades (**/facade/ — the import-linter-enforced boundary), Celery autoimport (**/tasks/), Django admin (**/admin/), and Temporal worker wiring (**/temporal/).
webjunkie
added
the
skip-agent-review
There was a problem hiding this comment.
Pull request overview
Adds a new devex Semgrep rule to prevent introducing new eager, top-level relative imports in __init__.py (a common source of import-time cost and import cycles), while grandfathering existing occurrences via paths.exclude and exempting specific “boundary” directories by path.
Changes:
- Introduce
no-init-reexports(severityERROR) scanning**/__init__.pyfor column-0from .…imports. - Exempt known intentional boundaries (e.g.
**/models/**,**/facade/**,**/tasks/__init__.py,**/admin/__init__.py,**/temporal/**) and freeze the current baseline via explicit excludes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # existing ruff exemptions (settings star-imports, generated stubs) | ||
| - 'posthog/settings/**' | ||
| - '**/generated/**' |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dae3f09acc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Column-0 anchor: only flag eager, top-level re-exports. Indented ones (TYPE_CHECKING, | ||
| # try/except ImportError, function-local) are already lazy and intentionally skipped. | ||
| pattern-regex: '(?m)^from\s+\.' |
There was a problem hiding this comment.
When a new __init__.py wraps a re-export in a module-level block such as try: from .heavy import Heavy or if optional: from .heavy import Heavy, this regex misses it solely because the import is indented, even though Python still executes it during package import and pays the same eager-import/cycle cost the rule is trying to ratchet against. Either extend the rule to distinguish module-level blocks from truly lazy locations, or narrow this comment so try/except ImportError is not treated as an allowed lazy pattern.
Useful? React with 👍 / 👎.
Problem
Eager top-level re-exports in
__init__.py(from .submodule import Thing) pull a package's whole submodule subtree onto the import path. Every entrypoint (web, celery, temporal, CI, shell) pays that import cost, and cross-package re-exports are a frequent source of import cycles. House style is deep imports from the defining module, but nothing stops new eager re-exports from landing.Changes
A new
severity: ERRORsemgrep rule in thesemgrep-devexset, run as a pure ratchet (not a burn-down):__init__.py.paths.exclude. There is no cleanup obligation. The list just freezes today's set so the pattern stops spreading, and shrinks only if someone happens to convert one.(?m)^from\s+\.), so already-lazy re-exports (TYPE_CHECKING,try/except ImportError, function-local) are indented and never flagged. No allowlist to maintain for them.**/models/), facades (**/facade/, the import-linter-enforced boundary), Celery autoimport (**/tasks/), Django admin (**/admin/), Temporal worker wiring (**/temporal/).No CI wiring needed. The
semgrep-devexjob already scans.semgrep/rules/devex/.Note
This is a file-level ratchet, matching the existing
no-environments-api-urls-frontendrule. It stops the pattern reaching new packages, but a grandfathered file is not frozen at its current occurrence count. Occurrence-level guarding would needsemgrep --baseline-commitin the shared devex job, which is out of scope here.How did you test this code?
I (Claude) ran the rule locally with semgrep 1.161.0 over the exact CI scope (
bin common ee frontend packages posthog products). No manual product testing applies, this is a lint rule.from .x import Yin a non-exempt path: flagged.TYPE_CHECKING/ function-local / excluded-path re-exports: not flagged..semgrep/rules/devex/set still parses (semgrep --validate).format:yamlpass with the regex and anchor intact.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Julian directed this; I (Claude) built it. It started as an exploration of whether to lint against
__init__.pyre-exports. I measured the scope first (138 re-exporting__init__.pyrepo-wide, and confirmed the nesting is shallow, mostly single-level), then landed on a ratchet rather than a burn-down at Julian's direction: existing usage grandfathered, new usage blocked, curated boundaries exempt by path.Two decisions worth flagging for review:
__all__. The harmful__all__only exists to silence F401 on eager re-exports, so it is already capped indirectly by this rule. A direct__all__ban would wrongly hit the lazy PEP 562__getattr__pattern we want to encourage.No repo skills were invoked.
/simplifywas run on the diff (clean).