feat: opt-in bundling of pure-Python third-party dependencies - #555
feat: opt-in bundling of pure-Python third-party dependencies#555tinovyatkin wants to merge 61 commits into
Conversation
Adds a --bundle-third-party mode (also cribo.toml bundle-third-party and CRIBO_BUNDLE_THIRD_PARTY) that inlines pure-Python site-packages dependencies into the bundle, similar to how esbuild bundles node_modules. Packages shipping native extension artifacts (.so/.pyd) anywhere in their top-level package directory are automatically kept external as a whole and still emitted into requirements.txt, acting as an automatic equivalent of esbuild's 'external' option. known_third_party remains a manual keep-external escape hatch.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds opt-in third-party bundling through TOML, environment, and CLI configuration. The resolver bundles eligible pure-Python virtualenv packages, keeps unsupported packages external, and updates requirements generation and integration tests. ChangesThird-party bundling
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant CriboCLI
participant Config
participant ModuleResolver
participant SitePackages
participant Requirements
CriboCLI->>Config: enable bundle_third_party
Config->>ModuleResolver: provide bundling configuration
ModuleResolver->>SitePackages: resolve candidate package
SitePackages-->>ModuleResolver: return package path
ModuleResolver->>ModuleResolver: scan package policy
ModuleResolver->>Requirements: classify bundled and external imports
Requirements-->>CriboCLI: emit external dependencies only
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Ecosystem Test Results📋 Test StatusTest Summary:
📈 Benchmark Results📊 View detailed benchmark report 📦 Package Bundling Metrics
Benchmark metrics are tracked via Bencher.dev 📊 View detailed performance trends and comparisons on the Bencher dashboard. Generated by ecosystem-tests workflow |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cribo/src/config.rs`:
- Line 122: Update Config::load and the Config combine flow so
bundle-third-party remains an Option<bool> while each TOML layer is merged,
preserving lower-precedence values when the key is absent; apply the false
default only after all configuration layers are combined, before constructing
the final Config.
- Around line 491-507: Document every newly added function with Rust doc
comments: add documentation for test_bundle_third_party_config in
crates/cribo/src/config.rs (lines 491-507), bundle_third_party_resolver and each
new test function in crates/cribo/src/resolver.rs (lines 2764-2870), and both
new test functions in crates/cribo/tests/test_cli_stdout.rs (lines 381-530).
In `@crates/cribo/src/resolver.rs`:
- Around line 786-805: Update the relative-import resolution flow around
resolve_relative_to_absolute_module_name and path_to_module_parts so module-part
discovery also searches virtualenv site-packages roots from
get_virtualenv_site_packages_search_directories. Ensure relative imports within
bundled site-packages packages derive an absolute module name and are resolved
by resolve_in_site_packages_for_bundling, then add coverage for a bundled
package whose __init__.py imports a helper relatively.
- Around line 1234-1255: Update directory_contains_native_extensions to avoid
recursively traversing directory symlinks: track canonicalized directories
already visited, and only enqueue symlinked directories when their resolved path
remains within the package root; otherwise stop traversal safely. Preserve
native-extension detection for regular files and prevent cycles or repeated
traversal from consuming unbounded resources.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 678e6764-a413-489b-a15d-c0fdf5cedef5
⛔ Files ignored due to path filters (1)
docs/resolution.mdis excluded by!**/docs/**
📒 Files selected for processing (5)
crates/cribo/src/config.rscrates/cribo/src/main.rscrates/cribo/src/orchestrator.rscrates/cribo/src/resolver.rscrates/cribo/tests/test_cli_stdout.rs
|
Bencher Report| Project | cribo |
| Branch | feat/bundle-third-party |
| Testbed | ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| build_dependency_graph | 📈 view plot 🚷 view threshold | 627.36 ns(-83.99%)Baseline: 3,918.97 ns | 13,586.32 ns (4.62%) |
| bundle_simple_project | 📈 view plot 🚷 view threshold | 1,407,600.00 ns(-11.71%)Baseline: 1,594,355.94 ns | 5,219,043.50 ns (26.97%) |
| resolve_module_path | 📈 view plot 🚷 view threshold | 111.17 ns(+0.87%)Baseline: 110.21 ns | 148.72 ns (74.75%) |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in mode to inline pure-Python third-party (virtualenv/site-packages) dependencies into the generated bundle while automatically keeping native-extension distributions external, and updates requirements emission accordingly.
Changes:
- Introduces
--bundle-third-party,bundle-third-party = true, andCRIBO_BUNDLE_THIRD_PARTYto enable third-party bundling. - Extends resolver classification + resolution to allow bundling of eligible site-packages modules, including a native-artifact scan to keep such packages external.
- Adjusts
requirements.txtgeneration (in the new mode) to omit third-party imports whose sources were inlined; adds CLI/integration tests and documentation updates.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/resolution.md | Documents the opt-in third-party bundling behavior and configuration knobs. |
| crates/cribo/tests/test_cli_stdout.rs | Adds CLI integration tests covering bundled pure-Python deps and external native deps. |
| crates/cribo/src/resolver.rs | Implements opt-in site-packages bundling, native artifact detection + caching, and resolution fallback logic. |
| crates/cribo/src/orchestrator.rs | Skips requirement entries for dependencies that were inlined under --bundle-third-party. |
| crates/cribo/src/main.rs | Adds CLI flag wiring for --bundle-third-party. |
| crates/cribo/src/config.rs | Adds config/env support and a config-loading test for bundle-third-party. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![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: fba87be0e3
ℹ️ 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".
- rustfmt: format config.rs and resolver.rs (CI static-analysis failure) - config: keep bundle-third-party as Option<bool> through layered config merging so higher-precedence files that omit the key no longer clobber lower-precedence values; add Config::bundle_third_party() accessor - resolver: resolve relative imports inside bundled site-packages modules by including environment roots in path_to_module_parts - resolver: apply known_third_party escape hatch to submodules of the configured package roots (prefix matching) - resolver: honor CONDA_PREFIX when locating environment site-packages, consistent with RequirementResolver - resolver: keep packages that read their own installed distribution metadata at runtime (importlib.metadata, importlib_metadata, pkg_resources) external, since dist-info is unavailable in a bundle - resolver: make the package scan symlink-safe and conservative - directory symlinks and unreadable entries keep the package external - tests: cover relative imports (unit + end-to-end execution), known_third_party submodules, metadata-dependent packages, symlink cycles, and config layer precedence; add doc comments to new tests - docs: document the refined external-detection rules
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7875b88899
ℹ️ 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".
| if self.config.bundle_third_party() { | ||
| search_dirs.extend(self.get_virtualenv_site_packages_search_directories(None)); |
There was a problem hiding this comment.
When a project-local .venv is beneath the entry directory, these site-packages roots are appended after the entry root, so path_to_module_parts strips a bundled dependency path against the project directory first and derives a name such as .venv.lib.python3.12.site-packages.pkg instead of pkg. Relative imports inside that dependency then fail to resolve. Fresh evidence beyond the earlier relative-import report is that the newly added fallback root is present but loses to its parent directory; prefer the most specific matching root and cover the normal project-local layout with the required snapshot fixture.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| // Conservative: a package that cannot be fully inspected stays external | ||
| return true; | ||
| }; | ||
| for entry in entries.flatten() { |
There was a problem hiding this comment.
When read_dir opens successfully but yields an error for an individual entry, flatten() silently discards that entry and the scan may return false without inspecting it. A transient filesystem error or unreadable entry can therefore hide a native extension or metadata-dependent source file, causing the package to be bundled despite the stated conservative policy; handle each iterator error by keeping the package external.
Useful? React with 👍 / 👎.
📊 Source Code Metrics (this PR vs
|
| File | Cyclomatic | Cognitive | Functions | LLOC | MI |
|---|---|---|---|---|---|
| crates/cribo/src/resolver.rs | 1125 (main: 435) 🔴 | 1104 (main: 372) 🔴 | 203 (main: 90) 🔴 | 1960 (main: 789) 🔴 | 0 ⚪ |
| crates/cribo/src/graph_builder.rs | 407 (main: 402) 🔴 | 317 (main: 313) 🔴 | 71 ⚪ | 658 (main: 646) 🔴 | 0 ⚪ |
| crates/cribo/src/orchestrator.rs | 312 (main: 240) 🔴 | 356 (main: 259) 🔴 | 39 (main: 36) 🔴 | 616 (main: 487) 🔴 | 0 ⚪ |
| crates/cribo/src/visitors/import_discovery.rs | 189 (main: 156) 🔴 | 158 (main: 119) 🔴 | 34 (main: 30) 🔴 | 315 (main: 257) 🔴 | 0 ⚪ |
| crates/cribo/src/visitors/local_var_collector.rs | 62 (main: 61) 🔴 | 27 ⚪ | 26 (main: 25) 🔴 | 217 (main: 215) 🔴 | 2.34 (main: 2.73) 🔴 |
| crates/cribo/src/code_generator/module_transformer.rs | 433 (main: 432) 🔴 | 635 (main: 631) 🔴 | 22 ⚪ | 655 (main: 653) 🔴 | 0 ⚪ |
| crates/cribo/src/config.rs | 86 (main: 82) 🔴 | 50 (main: 48) 🔴 | 21 (main: 19) 🔴 | 145 (main: 131) 🔴 | 0 (main: 0.33) 🔴 |
| crates/cribo/src/code_generator/import_transformer/handlers/statements.rs | 75 (main: 72) 🔴 | 79 (main: 75) 🔴 | 15 ⚪ | 184 (main: 163) 🔴 | 0.74 (main: 2.41) 🔴 |
| crates/cribo/src/visitors/utils.rs | 62 (main: 21) 🔴 | 45 (main: 11) 🔴 | 11 (main: 6) 🔴 | 66 (main: 33) 🔴 | 10.32 (main: 26.05) 🔴 |
| crates/cribo/src/ast_builder/proxy_generator.rs | 36 ⚪ | 20 ⚪ | 10 ⚪ | 79 ⚪ | 6.28 (main: 6.29) 🔴 |
| crates/cribo/src/python/importlib_call.rs | 48 🆕 | 24 🆕 | 10 🆕 | 39 🆕 | 18.46 🆕 |
| crates/cribo/src/code_generator/import_transformer/handlers/dynamic.rs | 42 (main: 39) 🔴 | 35 (main: 46) 🟢 | 5 ⚪ | 48 (main: 46) 🔴 | 15.87 (main: 18.22) 🔴 |
| crates/cribo/src/analyzers/module_classifier.rs | 55 (main: 52) 🔴 | 105 (main: 103) 🔴 | 3 ⚪ | 96 (main: 94) 🔴 | 8.94 (main: 9.92) 🔴 |
| crates/cribo/src/code_generator/import_transformer/expr_rewriter.rs | 118 (main: 115) 🔴 | 218 (main: 216) 🔴 | 3 ⚪ | 222 ⚪ | 0 ⚪ |
| crates/cribo/src/code_generator/import_transformer/state.rs | 6 ⚪ | 0 ⚪ | 3 ⚪ | 3 ⚪ | 31.50 (main: 32.22) 🔴 |
| crates/cribo/src/code_generator/init_function/orchestrator.rs | 12 (main: 5) 🔴 | 5 (main: 1) 🔴 | 3 (main: 2) 🔴 | 36 (main: 14) 🔴 | 19.53 (main: 29.56) 🔴 |
| crates/cribo/src/code_generator/init_function/state.rs | 5 ⚪ | 0 ⚪ | 2 ⚪ | 2 ⚪ | 37.29 (main: 38.15) 🔴 |
| crates/cribo/src/code_generator/init_function/initialization.rs | 7 (main: 5) 🔴 | 5 (main: 3) 🔴 | 1 ⚪ | 17 (main: 13) 🔴 | 27.57 (main: 31.69) 🔴 |
| crates/cribo/src/main.rs | 19 (main: 18) 🔴 | 15 (main: 14) 🔴 | 1 ⚪ | 30 (main: 28) 🔴 | 24.09 (main: 25.01) 🔴 |
| crates/cribo/src/ast_builder/mod.rs | 1 ⚪ | 0 ⚪ | 0 ⚪ | 0 ⚪ | 43.52 (main: 44.56) 🔴 |
| crates/cribo/src/python/mod.rs | 1 ⚪ | 0 ⚪ | 0 ⚪ | 0 ⚪ | 66.92 (main: 69.47) 🔴 |
Generated by mehen v1.3.0 — the code quality watcher.
Extract shared sandbox construction and cribo invocation into helpers to resolve the SonarCloud duplicated-lines quality gate failure.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 445e1a82ec
ℹ️ 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".
…fallback - detect runtime distribution-metadata imports (importlib.metadata, importlib_metadata, pkg_resources) from the parsed AST instead of substring matching, so parenthesized/aliased/multiline import forms are recognized while comments and docstrings are not false positives - resolve static importlib.import_module relative imports inside bundled site-packages dependencies via the site-packages fallback - add unit coverage for both, including all import form variants
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd74fd1d0b
ℹ️ 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".
| match stmt { | ||
| Stmt::Import(import_stmt) => { |
There was a problem hiding this comment.
When a dependency uses the supported literal form import importlib; importlib.import_module("importlib.metadata").version("dist"), this visitor sees only the ordinary import importlib and never examines the call, so the package is bundled and its requirement/dist-info metadata is omitted; the resulting bundle raises PackageNotFoundError. Fresh evidence after the earlier metadata report is that the AST replacement handles only Import/ImportFrom, even though Cribo explicitly discovers static importlib.import_module calls; extend the detector to those calls and cover the case with an execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| if self.config.bundle_third_party() && classification.should_bundle() { | ||
| continue; |
There was a problem hiding this comment.
When an output directory already contains requirements.txt from a run without --bundle-third-party, enabling the flag can make every dependency hit this continue, producing empty requirements content. The writer's empty-content branch only logs and skips writing, so the old file remains and still lists distributions that are now bundled; remove or truncate the existing requirements file when the generated result is empty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cribo/src/resolver.rs`:
- Around line 1841-1847: Cache results from
get_virtualenv_site_packages_search_directories(None) in a new
RefCell<Option<Vec<PathBuf>>> field alongside virtualenv_packages_cache,
returning the cached roots on subsequent calls. Update all callers, including
path_to_module_parts, resolve_in_site_packages_for_bundling,
classify_import_uncached, and get_import_search_root, to reuse this cache while
preserving the existing directory discovery behavior.
- Around line 1292-1322: Update package_must_stay_external and its package
scanning logic to detect pure-Python packages that access adjacent runtime data
resources, including relative __file__-based reads and
importlib.resources.files(__package__). Return true for such packages so
--bundle-third-party leaves them external, while preserving existing
native-extension and metadata checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c0b7733-f72a-4981-b78f-1fcf33695bbf
⛔ Files ignored due to path filters (1)
docs/resolution.mdis excluded by!**/docs/**
📒 Files selected for processing (5)
crates/cribo/src/config.rscrates/cribo/src/main.rscrates/cribo/src/orchestrator.rscrates/cribo/src/resolver.rscrates/cribo/tests/test_cli_stdout.rs
- packages importing importlib.resources are kept external since their package data files are unavailable once inlined into a bundle; document __file__-relative data reads as an explicit limitation with the known_third_party escape hatch - cache resolved environment site-packages roots, which now sit on the per-import resolution hot path
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ada7b6b47e
ℹ️ 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".
importlib.import_module('.helper', package='pkg') now resolves its
package context, matching the positional form; covered by a visitor
unit test for both forms.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 604c73e181
ℹ️ 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".
| return module_file.is_file() && Self::python_file_requires_distribution(&module_file); | ||
| } | ||
|
|
||
| let package_dir = self.canonicalize_path(package_dir); |
There was a problem hiding this comment.
When a top-level package directory in site-packages is a symlink to a pure-Python source tree outside the registered roots, canonicalizing it here makes the scan follow the target and classify the package as bundleable; resolution also returns canonical target paths, so path_to_module_parts can no longer derive the package name and relative imports inside the package fail during discovery. Either keep top-level directory symlinks external or preserve their site-packages-relative identity, and cover the symlinked-package flow with an execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| let virtualenv_dirs = self.get_virtualenv_site_packages_search_directories(None); | ||
| self.locate_in_directories(module_name, &virtualenv_dirs) | ||
| .and_then(|(_, resolved)| resolved.bundle_path()) |
There was a problem hiding this comment.
When an active environment contains an editable install whose .pth file adds a source directory, Python can import that package but this fallback searches only for physical children of the site-packages directories. The package is therefore never classified or resolved for bundling, so --bundle-third-party silently leaves a common pure-Python installed dependency external; resolve imports using the selected environment's effective import paths and add an end-to-end snapshot for a .pth-based install.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 604c73e181
ℹ️ 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".
…v discovery - distributions whose RECORD lists native artifacts anywhere (e.g. a pure frontend package with a sibling _backend.so) keep every import they own external, using per-distribution ownership indexing - treat the importlib_resources backport like importlib.resources - auto-detect virtualenvs beside the entry directory (monorepo-root invocations), invalidating environment caches when the entry is set - only prefer path-derived module names when all components are valid Python identifiers, so a virtualenv inside the entry directory can no longer produce mangled module names like '.venv.lib....pkg'
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cribo/tests/test_cli_stdout.rs (1)
416-443: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftUse the generic bundling snapshot framework for the primary coverage.
Move the pure-Python, native-external, and entry-adjacent virtualenv scenarios into fixture directories with
main.pyfiles. Run them throughcrates/cribo/tests/test_bundling_snapshots.rswithINSTA_GLOB_FILTER. Keep a minimal subprocess test only for behavior that snapshots cannot validate.Also applies to: 450-498, 504-554, 556-588
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cribo/tests/test_cli_stdout.rs` around lines 416 - 443, The bundling scenarios currently covered by run_bundle_third_party_cribo and the related tests should be moved into fixture directories with main.py files and exercised through the generic snapshot framework in test_bundling_snapshots.rs using INSTA_GLOB_FILTER. Retain only a minimal subprocess test for behavior snapshots cannot validate, and remove redundant pure-Python, native-external, and entry-adjacent virtualenv coverage from test_cli_stdout.rs.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cribo/src/resolver.rs`:
- Around line 1736-1745: Update the RECORD-processing loop in the resolver to
parse each line with a quote-aware CSV parser instead of line.split(',').next(),
preserving the complete first path field before indexing and native-artifact
detection. Add a regression test covering a quoted RECORD path containing commas
and a .so or .pyd extension, verifying it is classified as a native artifact and
prevents bundling.
In `@crates/cribo/src/visitors/import_discovery.rs`:
- Around line 426-447: Add a Rust documentation comment directly above
extract_package_context describing that it accepts only string-literal package
contexts supplied through the second positional argument or the package= keyword
argument.
---
Outside diff comments:
In `@crates/cribo/tests/test_cli_stdout.rs`:
- Around line 416-443: The bundling scenarios currently covered by
run_bundle_third_party_cribo and the related tests should be moved into fixture
directories with main.py files and exercised through the generic snapshot
framework in test_bundling_snapshots.rs using INSTA_GLOB_FILTER. Retain only a
minimal subprocess test for behavior snapshots cannot validate, and remove
redundant pure-Python, native-external, and entry-adjacent virtualenv coverage
from test_cli_stdout.rs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69961818-7de6-4169-b7c1-9a87603d459c
⛔ Files ignored due to path filters (1)
docs/resolution.mdis excluded by!**/docs/**
📒 Files selected for processing (4)
crates/cribo/src/orchestrator.rscrates/cribo/src/resolver.rscrates/cribo/src/visitors/import_discovery.rscrates/cribo/tests/test_cli_stdout.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32f8243bb7
ℹ️ 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".
| if !source.contains("importlib") | ||
| && !source.contains("pkg_resources") | ||
| && !source.contains("__import__") | ||
| && !source.contains("__file__") | ||
| { | ||
| return false; |
There was a problem hiding this comment.
When a pure installed package performs standard package discovery such as pkgutil.iter_modules(__path__), its source contains none of this pre-filter's four tokens, and the AST detector also recognizes only __file__. The distribution is therefore bundled and removed from requirements, but the generated module has no installed package search path, so the preserved discovery returns no plugins or raises on the bare __path__ reference. Treat __path__ consumers as external or reproduce their runtime package path, and cover this with an execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| for import in discovered_modules[index].2.clone() { | ||
| mark(&import, &mut reachable, &mut queue); |
There was a problem hiding this comment.
When any module is dropped after a late metadata query, this reachability walk consumes the raw discovery name for a retained call such as importlib.import_module(".sub", "pkg"). Discovery queued that target as pkg.sub, but the stored import remains .sub, so mark cannot find it and prunes the still-needed submodule. Code generation then preserves the runtime call while the bundled parent distribution is omitted from requirements, causing an isolated bundle to fail; resolve these names with their recorded package context before marking reachability and add a combined regression snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| .ignore_import_bindings() | ||
| .collect_from_stmts(&func.body); | ||
| } | ||
| self.shadowed_names_stack.push(parameter_names); |
There was a problem hiding this comment.
When a parameter or body assignment shadows an imported importlib name, a definition-time expression such as def load(importlib=importlib.import_module("provider")): still evaluates the call in the enclosing scope. Fresh evidence beyond the addressed function-local shadowing cases is that this scope state is pushed before the source-order visitor traverses defaults, annotations, and decorators, so the valid static target is ignored and is neither bundled nor emitted as a requirement; apply the precomputed shadow set only while traversing the body and add an isolated execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| let metadata_file = dist_info_dir.join("METADATA"); | ||
| if let Some(metadata) = Self::read_metadata_sidecar(&metadata_file, incomplete) { | ||
| Self::index_distribution_metadata(&metadata, &mut distribution); |
There was a problem hiding this comment.
When a .dist-info directory exists but its mandatory METADATA file is absent, such as during a concurrent or damaged installation, read_metadata_sidecar treats NotFound as benign and this leaves an empty distribution record while the ownership index remains complete. A remaining RECORD can still associate the pure package with that empty record, allowing it to be bundled and removed from requirements while its name, Requires-Python, and Requires-Dist constraints are lost. Fresh evidence beyond the addressed unreadable-file case is that a missing primary metadata file follows the optional-sidecar path; mark this case incomplete and cover it with a targeted error-condition test.
AGENTS.md reference: AGENTS.md:L204-L208
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a6f7cded1
ℹ️ 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".
| if !source.contains("importlib") | ||
| && !source.contains("pkg_resources") | ||
| && !source.contains("__import__") | ||
| && !source.contains("__file__") | ||
| { |
There was a problem hiding this comment.
When --bundle-third-party encounters a package that loads bundled data with the standard pkgutil.get_data(__name__, "data.json") pattern, this pre-filter returns before parsing because the source contains none of these four tokens. The package is consequently inlined and removed from requirements even though its data file is not emitted, so the lookup fails in an isolated deployment. Detect pkgutil resource APIs or retain those packages, and cover the case with bundle and execution snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 0c818fe. pkgutil joined the blocked installed-package APIs (both import pkgutil and from pkgutil import get_data forms) and the cheap pre-filter now includes the pkgutil token, so pkgutil.get_data(__name__, "data.json") consumers stay external with their distribution in requirements. Covered by new detector cases in test_distribution_metadata_import_detection and the xfail_bundle_third_party_pkgutil_data fixture (requirements snapshot retains pkgutil-pkg; the data file is never emitted into the bundle). Thanks!
| Expr::Attribute(attribute) => Some(attribute.attr.as_str()), | ||
| Expr::Name(name) => Some(name.id.as_str()), | ||
| _ => None, |
There was a problem hiding this comment.
When a bundled module also references importlib or pkg_resources and invokes an unrelated callable whose final name is version, files, metadata, etc.—for example provider.version("provider")—this records the literal without verifying that the callee came from a metadata API. The name enters queried_distributions before import classification, forcing the matching provider to remain external and adding it to requirements even though no metadata query occurred. Track scope-aware imports and aliases rather than matching only the final spelling, with a snapshot for an unrelated same-named method.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 0c818fe. queried_distribution_names now resolves callees before recording: an alias pass collects the module's import bindings (including function-local and as-renamed ones), and a call records names only when its callee canonicalizes to one of the metadata query functions (importlib.metadata.*, importlib_metadata.*, pkg_resources.get_distribution/require). provider.version("provider") and similar same-named methods record nothing. Covered by the extended test_queried_distribution_names_collection (module-alias positive plus provider.version/provider.metadata/provider.files negatives) and the bundle_third_party_unrelated_version_method fixture, whose requirements snapshot stays empty while provider is inlined. Thanks!
| if let Expr::Call(call) = expr { | ||
| match self.dynamic_import_kind(&call.func) { |
There was a problem hiding this comment.
When a pure package uses importlib.util.find_spec("provider.backend") to detect one of its own optional submodules, this call is ignored because only dynamic-import callees are inspected here. Cribo can then inline the provider and backend, remove the installed distribution, and represent them as namespaces that are not registered with Python's import machinery; the preserved find_spec call therefore fails or reports the backend absent even though the original installation found it. Keep packages that introspect bundled module specs external or emit real import specs, with an execution snapshot for this capability-detection pattern.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 0c818fe. importlib.util is now a blocked API (import importlib.util, from importlib import util, from importlib.util import find_spec) and find_spec calls are additionally detected at the expression level regardless of qualification, since inlined modules are not registered with the import machinery and spec probes would misreport them. Covered by detector cases in test_distribution_metadata_import_detection and the xfail_bundle_third_party_find_spec_probe fixture (importlib.util.find_spec("spec_pkg.backend") keeps spec-pkg external and in requirements). Thanks!
- keep pkgutil resource consumers external (pkgutil.get_data reads package data files that bundles do not ship) - keep import-spec introspection consumers external (importlib.util find_spec misreports inlined modules) - resolve metadata-query callees through imports and aliases before recording queried distribution names, so unrelated callables sharing a final name (provider.version) record nothing - reject Requires-Python specifiers excluding a patch inside the target minor range (!=3.10.5 passes both endpoint probes)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c818fe435
ℹ️ 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".
…ging - metadata-query callees resolve through per-scope import bindings and shadow sets, so a function-local rebinding (import json as md) no longer hides a module-level query from requirement collection - queried requirement literals are preserved verbatim, so extras and version specifiers from pkg_resources.require survive into requirements.txt - same-marker requirement duplicates now intersect their constraints (module-map overrides combine with bundled Requires-Dist entries)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 707395ecf3
ℹ️ 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".
| // arguments cannot be safely discarded (e.g. package=touch()) are left | ||
| // for runtime and must not be recorded as static imports | ||
| if self.is_static_importlib_call(call) | ||
| && crate::python::importlib_call::arguments_safely_discardable(call) |
There was a problem hiding this comment.
When entry or first-party code reaches a dependency only through importlib.import_module("provider", package=__package__) (or a side-effectful package=touch()), this condition rejects the call, so no discovered import or external target reaches requirements generation. The transformer correctly preserves the runtime call, but the provider is neither bundled nor emitted in requirements.txt, causing an isolated bundle to fail. Fresh evidence after the argument-evaluation fix is this target-only form; the existing fixture also imports its target normally and therefore masks the missing dependency tracking. Record the literal target as a preserved external dependency even when the call cannot be rewritten, and add an execution/requirements snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5b56013 — by solving rather than externalizing. import_module("provider", package=touch()) has a statically known absolute target, so the target is now BUNDLED and the call rewritten to (touch(), <bundled module access>)[1], preserving the extra argument's evaluation, side effects, and exceptions (CPython evaluates but ignores package for absolute names). Discovery, graph building, codegen, and the detector share the new shape helpers. Calls statically known to raise TypeError are preserved verbatim (they never import, so no dependency); only opaque unpacked shapes (*args/**kwargs) keep the literal target as a preserved runtime dependency that reaches requirements. Covered by the bundle_third_party_importlib_evaluable_package fixture: the target is reached ONLY through this call form, the execution snapshot shows "context evaluated" followed by the bundled value, and requirements stay empty. Thanks!
| use ruff_python_ast::Stmt; | ||
|
|
||
| /// Canonical dotted paths of the query functions that take a distribution name. | ||
| const QUERY_FUNCTIONS: [&str; 12] = [ |
There was a problem hiding this comment.
When entry code imports a pure provider and calls a global metadata API such as importlib.metadata.packages_distributions() or distributions(), this allowlist records nothing because those APIs take no distribution-name argument. The provider is consequently bundled and removed from requirements even though its dist-info is not emitted, so the preserved enumeration no longer sees the provider in an isolated deployment. Detect global metadata enumeration and retain the affected distributions, with an execution/requirements snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5b56013. Global enumeration (importlib.metadata.distributions(), packages_distributions(), and the importlib_metadata twins) is detected through the same scoped callee resolution as name-taking queries, and sets a resolver-wide flag under which every import owned by any installed distribution stays external — an enumeration observes the whole environment, and inlined distributions would vanish from its results since their dist-info is not emitted. Covered by test_global_distribution_enumeration_detection (incl. an unrelated provider.distributions() negative) and the xfail_bundle_third_party_packages_distributions fixture whose requirements snapshot retains provider. This one is genuinely constrained by the single-file output today; a future solving path is embedding bundled distributions' metadata behind a sys.meta_path DistributionFinder, which would let enumeration see bundled packages — noted as follow-up work rather than done here. Thanks!
| /// mention the APIs are not. | ||
| fn python_source_blocks_bundling(source: &str) -> bool { | ||
| // Cheap pre-filter: skip parsing sources that cannot reference the APIs | ||
| if !source.contains("importlib") |
There was a problem hiding this comment.
When a pure dependency accesses its own module object through a standard pattern such as SELF = sys.modules[__name__], this prefilter skips AST inspection because the source contains none of its recognized tokens. Cribo then bundles the dependency and removes its requirement, but the generated SimpleNamespace is never registered in sys.modules; the rewritten __name__ therefore indexes a missing entry and raises KeyError (and membership checks incorrectly return false). Keep sys.modules consumers external or register bundled module namespaces, and add an isolated execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 6f0c1e2 — by solving rather than externalizing. Modules accessing their own sys.modules entry (sys.modules[__name__], __name__ in sys.modules, .get(__name__), incl. from sys import modules bindings) are wrapped, and their init registers the module object in sys.modules under its original name (with __spec__) before the body executes — mirroring Python's import machinery — so the pattern now works inside bundled packages. Registration is deliberately scoped to this self-access pattern: registering every bundled module shadows installed distributions whose native extensions re-import their package mid-init (the pyyaml ecosystem scenario caught exactly that during development). Covered by the bundle_third_party_sys_modules_self fixture: the package is bundled (requirements empty) and the isolated execution snapshot shows the self-reference resolving plus "self_pkg" in sys.modules being true. Thanks!
- import_module("pkg", package=touch()) now bundles its statically known
target and rewrites to (touch(), <module access>)[1], preserving the
evaluation, side effects, and exceptions of the extra argument that
CPython ignores for absolute names; discovery, graph building, code
generation, and the unbundlable-pattern detector share the new shape
helpers (evaluable_package_argument, statically_raises_type_error,
has_opaque_arguments)
- calls statically known to raise TypeError are preserved verbatim and
no longer force their caller external; only opaque unpacked argument
shapes do (with the literal target recorded as a preserved runtime
dependency for requirements)
- global metadata enumeration (importlib.metadata.distributions,
packages_distributions) keeps every installed distribution external:
bundled distributions would vanish from its results
Modules accessing their own sys.modules entry (sys.modules[__name__], __name__ in sys.modules, .get(__name__)) are wrapped and their init registers the module object in sys.modules under its original name before the body executes, mirroring Python's import machinery, so the pattern works in bundled third-party packages instead of forcing them external. Registration is scoped to exactly this self-access pattern: registering every bundled module would shadow installed distributions whose native extensions re-import their package mid-initialization (caught by the pyyaml ecosystem scenario).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f0c1e2159
ℹ️ 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".
| && import_from | ||
| .names | ||
| .iter() | ||
| .any(|alias| alias.name.as_str() == "modules" && alias.asname.is_none()) |
There was a problem hiding this comment.
When a bundled dependency uses from sys import modules as loaded followed by loaded[__name__], this condition ignores the alias, so accesses_own_sys_modules_entry returns false and initialization never registers the wrapper in sys.modules. The lookup then raises KeyError after the distribution has been removed from requirements; fresh evidence beyond the addressed unaliased sys.modules case is the explicit alias.asname.is_none() restriction here. Resolve imported aliases (and import sys as ...) and add an isolated execution snapshot for the aliased form.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 09b4eba. accesses_own_sys_modules_entry now resolves aliases through a body-wide pre-pass: import sys as system binds system to sys, and from sys import modules as loaded (aliased or not) binds the modules mapping, so loaded[__name__] and system.modules-based self-access register the wrapper too. Covered by the bundle_third_party_sys_modules_alias fixture, which exercises both alias forms (loaded[__name__] in the package init and __name__ in system.modules in a submodule): the package bundles (requirements empty) and the isolated execution snapshot shows both resolving. Thanks!
| self.binding_scopes | ||
| .push(collect_scope_bindings(&function_def.body)); | ||
| self.shadow_scopes.push(shadows); | ||
| ruff_python_ast::visitor::walk_stmt(self, stmt); |
There was a problem hiding this comment.
When a function-local name shadows an outer metadata alias, as in def load(md=md.version("provider")), this pushes the function's bindings before walking the whole definition, even though defaults, decorators, and annotations execute in the enclosing scope. The valid metadata query is therefore missed, so a metadata-only provider is omitted from requirements and fails in an isolated deployment; traverse definition-time expressions before installing the body scope and add a requirements/execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 950df76. The scanner's FunctionDef/ClassDef handling now visits definition-time expressions — decorators, parameter defaults and annotations, return annotations, and class bases/keywords — in the ENCLOSING scope before installing the body scope, matching Python evaluation semantics, so def load(md=md.version("provider")) records the query even though the parameter shadows md inside the body. Covered by the extended test_queried_distribution_names_collection (default-expression positive alongside the body-shadowing behavior) and the xfail_bundle_third_party_metadata_default_query fixture, whose requirements snapshot retains provider while the isolated run deterministically reports the missing metadata. Thanks!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 950df76ba7
ℹ️ 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".
| self.binding_scopes | ||
| .push(collect_scope_bindings(&class_def.body)); | ||
| self.shadow_scopes | ||
| .push(collect_scope_shadows(&class_def.body)); |
There was a problem hiding this comment.
When a class body uses an enclosing metadata alias before assigning or importing the same name later, such as class C: version = md.version("provider"); md = custom, the first lookup still resolves to the enclosing md because class namespaces execute in source order. Precomputing all class-body bindings and shadows here incorrectly hides that valid query from the start, so a metadata-only provider can be omitted from requirements.txt and fail in an isolated bundle. Track class bindings in execution order and add a requirements/execution snapshot for the use-before-rebinding case.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in f82b5ce. Class bodies are now scanned in execution order: the class scope starts empty and each statement's bindings/shadows are added AFTER visiting it, so version = md.version("provider") before md = ... resolves to the enclosing alias while later uses see the rebinding. Class scopes are also now correctly excluded from the lexical lookup of functions nested inside them (Python semantics). Covered by the extended scanner unit test (class-order-name recorded, class-hidden-name not) and the xfail_bundle_third_party_metadata_class_order fixture whose requirements snapshot retains provider. Thanks!
| let record_file = dist_info_dir.join("RECORD"); | ||
| if let Some(record) = Self::read_metadata_sidecar(&record_file, incomplete) { |
There was a problem hiding this comment.
When a .dist-info installation lacks RECORD and also has no top_level.txt or Import-Name declaration—for example, a system-managed or damaged installation—this treats the missing file as benign and leaves the index complete without associating the package with its distribution. Under --bundle-third-party, the package can then be bundled while bundled_distribution_requirements finds no owner, dropping its Requires-Dist constraints and potentially missing sibling native artifacts. Separately from the already-reported missing METADATA case, this RECORD branch remains optional; treat missing ownership data conservatively and cover it with a targeted error-condition test.
AGENTS.md reference: AGENTS.md:L204-L208
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in f82b5ce — by solving rather than externalizing. A dist-info lacking RECORD, top_level.txt, and Import-Name still names its distribution, so the conventional import root is now inferred from the underscore-normalized name (exactly like egg-info indexing already did): ownership associates and Requires-Dist constraints survive. Only a metadata directory that yields no name at all marks the ownership index incomplete (unknowable ownership → conservative). Covered by test_bundled_distribution_requirements_infer_root_without_record and the bundle_third_party_bare_dist_info fixture: the package bundles and the requirements snapshot carries hidden-dep>=1 from the bare dist-info. Thanks!
| if let Expr::Name(name) = expr | ||
| && name.id.as_str() == "__file__" |
There was a problem hiding this comment.
When a pure package locates resources or derives package context through its standard import globals, such as Path(__spec__.origin).with_name("data.json") or __spec__.parent, this detector recognizes only __file__ and allows the package to be bundled. The preserved expression then observes the generated bundle's __spec__ (often None when run as a script), rather than the installed package's spec, causing an exception or resolving the wrong path after the distribution and its assets have been removed. Treat __spec__ location consumers like __file__ consumers and add an isolated execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in f82b5ce. __spec__ references now block bundling exactly like __file__: origin and submodule_search_locations describe the installed on-disk layout, which a single-file bundle cannot reproduce, so such consumers stay external with their distribution in requirements. Covered by detector unit cases and the xfail_bundle_third_party_spec_origin fixture (Path(__spec__.origin).with_name("data.json")), whose requirements snapshot retains spec-origin-pkg. Thanks!
| || import_aliases.contains(name.id.as_str()) | ||
| || assigned_aliases.contains(name.id.as_str()) | ||
| } | ||
| _ => false, |
There was a problem hiding this comment.
When a dependency wraps an import callable before assigning it, for example load = functools.partial(import_module, package=__package__) followed by load(dynamic_name), the assignment value is an Expr::Call and this fallback declines to record load as an undiscoverable import alias. Neither discovery nor the later detector recognizes the call, so --bundle-third-party can inline and remove the distribution without bundling the dynamically selected submodule, causing the isolated bundle to fail at runtime. Conservatively track import callables passed through higher-order wrappers and add a bundling/execution snapshot for the partial-based form.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in f82b5ce. record_assigned_import_module_aliases now tracks higher-order wrappers conservatively: when an assignment's value is a call receiving an import callable as any positional or keyword argument (e.g. load = functools.partial(import_module, package=__package__)), the assigned name is recorded as an undiscoverable import alias, so subsequent load(dynamic_name) calls block bundling and the distribution stays external. Covered by the xfail_bundle_third_party_partial_import_module fixture whose requirements snapshot retains partial-pkg. Thanks!
| "__spec__", | ||
| ast_builder::expressions::none_literal(), | ||
| )); | ||
| state.body.push(ast_builder::statements::assign( |
There was a problem hiding this comment.
When a self-inspecting bundled module raises during initialization and the importing code catches that exception, this registration remains in sys.modules and self.__initializing__ remains true because cleanup occurs only on the successful finalization path. Python's import machinery removes a module whose execution failed, so a later retry should execute it again; the generated guard instead returns the stale, partially initialized namespace without raising. Wrap registered initialization with failure cleanup that resets the guard and removes the entry when it still points to self, and add an execution snapshot covering a caught failure followed by a retry.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 4fb495b. A self-registering init now wraps everything after its prologue in try/except BaseException that resets __initializing__, removes the sys.modules entry when it still points to self, and re-raises — mirroring Python's failed-import semantics, so a caught failure leaves no stale entry and a retry re-executes the module. Covered by the sys_modules_failed_init fixture: its execution snapshot asserts "helper" in sys.modules is false after the caught failure and that a second import raises again rather than returning the partial namespace. Thanks!
A registered init now wraps everything after its prologue in try/except BaseException that resets __initializing__, removes the sys.modules entry when it still points to self, and re-raises — mirroring Python's failed-import semantics so a caught failure retries instead of returning a stale partial namespace.
…are dist-info - __spec__ references block bundling like __file__: origin and search locations describe the installed layout - import callables passed through higher-order wrappers (functools.partial(import_module, ...)) are tracked as undiscoverable import aliases - the metadata-query scanner resolves class-body bindings in source order (class namespaces do not hoist) and excludes class scopes from nested functions' lexical lookup - a dist-info without RECORD/top_level/Import-Name infers its import root from the distribution name (like egg-info indexing) so ownership and Requires-Dist constraints survive; a nameless metadata directory marks the ownership index incomplete instead
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f82b5cebec
ℹ️ 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".
| const ENUMERATION_FUNCTIONS: [&str; 4] = [ | ||
| "importlib.metadata.distributions", | ||
| "importlib.metadata.packages_distributions", | ||
| "importlib_metadata.distributions", | ||
| "importlib_metadata.packages_distributions", |
There was a problem hiding this comment.
Fresh evidence after the addressed global-enumeration case is pkg_resources.working_set: when first-party code iterates it and also imports a pure provider, this allowlist does not set enumerates_distributions because it recognizes only the importlib.metadata call forms. The provider is then bundled and omitted from requirements, so the preserved iteration no longer observes its distribution in an isolated deployment; recognize legacy working-set enumeration and cover it with an execution/requirements snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5af0e11. Legacy enumeration is now detected as OBJECT access, not just calls: any expression resolving through the scoped bindings to pkg_resources.working_set (or pkg_resources.Environment) sets the enumeration flag, keeping every installed distribution external. Covered by the xfail_bundle_third_party_working_set fixture (a never-called function iterating working_set): the requirements snapshot retains provider. Thanks!
| let stmt_bindings = collect_scope_bindings(statement); | ||
| let stmt_shadows = collect_scope_shadows(statement); | ||
| if let Some(scope) = self.scopes.last_mut() { | ||
| scope.bindings.extend(stmt_bindings); | ||
| scope.shadows.extend(stmt_shadows); |
There was a problem hiding this comment.
Fresh evidence after the class-order fix is a conditional binding such as class C: if False: md = custom; value = md.version("provider"): Python never creates the class-local md, so the later lookup reaches the enclosing metadata alias, but this code unconditionally merges every binding found inside the if statement after visiting it. The real query is therefore missed and a metadata-only provider can be omitted from requirements; update class-scope state only for bindings known to execute, conservatively retain possible outer resolution across branches, and add an execution/requirements snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5af0e11. Class-scope state now updates only for bindings known to execute: assignments, imports, and def/class statements at class-body top level. Bindings nested inside conditional statements (if/try/loops) neither rebind nor shadow, so outer alias resolution is conservatively retained across branches — over-collection being the safe direction. (Function scopes keep body-wide hoisting: a conditional function-local binding genuinely makes the name local throughout.) Covered by the xfail_bundle_third_party_metadata_conditional_class fixture (if False: md = str before the query), whose requirements snapshot retains provider. Thanks!
| current.iter().cloned().chain(additional).collect(); | ||
| existing.version_or_url = Some(VersionOrUrl::VersionSpecifier(combined)); | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
Fresh evidence beyond the addressed direct/transitive specifier merge is a module-map entry such as dep = "dep @ https://host/dep.whl" combined with a bundled distribution's Requires-Dist: dep<2: the direct URL is recorded first, this fallback silently discards the incoming version constraint, and the generated requirements can install a 2.x wheel that normal dependency resolution would reject. Preserve or explicitly validate the constraint instead of dropping it, with a requirements snapshot for the URL-plus-specifier combination.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5af0e11. PEP 508 cannot express a direct URL and a version specifier on one requirement line, and emitting them as two lines makes the file self-conflicting — so the URL (the artifact the environment actually resolved) wins, and the dropped constraint is now explicitly reported via a warning naming the requirement, the URL, and the unenforced constraint, in both merge directions. Covered by the xfail_bundle_third_party_module_map_url_constraint fixture (module-map URL + bundled Requires-Dist: external-dep<2), whose requirements snapshot shows the URL requirement retained. Thanks!
| let inferred = distribution.name.cow_replace('-', "_").into_owned(); | ||
| distribution.declared_prefixes.insert(inferred); |
There was a problem hiding this comment.
Fresh evidence after the absent-RECORD response is a distribution whose project and import names differ, such as beautifulsoup4 providing bs4: underscore-normalizing the distribution name claims only beautifulsoup4, so the real package remains unowned even though the index is considered complete. Under --bundle-third-party, bs4 can then be bundled without propagating that distribution's Requires-Dist constraints or distribution-wide native-artifact policy; when no ownership declaration or file listing exists, keep unmatched packages external rather than treating the name-derived guess as authoritative, and cover the mismatched-name layout with a targeted test.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5af0e11. Name inference is now verified against the filesystem: the underscore-normalized project name must match a package directory or module file beside the metadata, otherwise (the beautifulsoup4→bs4 case) ownership is unknowable and the index is marked incomplete, keeping the root's unmatched packages external instead of claiming a wrong prefix. The verification applies to dist-info and both egg-info forms, and egg-info inference now runs after installed-files.txt indexing so declared ownership always takes precedence. The bundle_third_party_bare_dist_info fixture still bundles (its name matches the module beside it), and test_bundled_distribution_requirements_infer_root_without_record covers the carried constraints. Thanks!
| if let Ok(Some(package_path)) = bundler.resolver.resolve_module_path(package) { | ||
| let level = module_name.chars().take_while(|&c| c == '.').count() as u32; | ||
| let name_part = module_name.trim_start_matches('.'); | ||
|
|
||
| bundler.resolver.resolve_relative_import_from_package_name( | ||
| level, | ||
| if name_part.is_empty() { | ||
| None | ||
| } else { | ||
| Some(name_part) | ||
| }, | ||
| package, | ||
| ) | ||
| } | ||
| } else { | ||
| module_name.to_owned() | ||
| } | ||
| bundler | ||
| .resolver | ||
| .resolve_relative_to_absolute_module_name( |
There was a problem hiding this comment.
With the newly supported keyword form importlib.import_module(name=".sub", package="pkg.mod"), when pkg.mod resolves to a regular module rather than a package, this path-based helper removes mod and rewrites the call as access to pkg.sub. Python instead resolves the requested name to pkg.mod.sub and raises because pkg.mod has no package path, so the bundle can silently turn a caught import failure into a successful import; resolve relative names from the literal package string as importlib does and preserve calls whose anchor is not a package, with an execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5af0e11. Relative names now resolve verbatim from the literal package string, exactly like CPython's importlib._bootstrap._resolve_name (shared helper resolve_relative_name): import_module(".sub", "pkg.mod") targets pkg.mod.sub with no path-based module-vs-package adjustment, so a plain-module anchor yields a name that is not bundled, the call stays preserved, and the runtime ImportError survives; anchors with too few components for the level also preserve the call (CPython raises ImportError). Covered by the bundle_third_party_importlib_module_anchor fixture: the execution snapshot shows "anchor is not a package" while the genuinely bundled sibling anchor_pkg.sub is unaffected. Thanks!
| ast_builder::expressions::name(SELF_PARAM, ExprContext::Load), | ||
| )); |
There was a problem hiding this comment.
When a self-inspecting bundled module deliberately replaces itself with sys.modules[__name__] = replacement, this prologue registers the wrapper and the preserved module body overwrites that entry, but successful initialization still returns self; transformed importers therefore receive the wrapper while normal Python returns replacement, and the two import paths expose different objects and attributes. For registered wrappers, honor the final sys.modules entry on successful completion (including parent attachment) and add an execution snapshot for a module that replaces itself.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5af0e11 — by solving. A registered init now returns _sys.modules.get(self.__name__, self) on successful completion, mirroring how CPython's _load re-reads sys.modules after execution, so a deliberate sys.modules[__name__] = replacement propagates to importers and parent attachment (the caller assigns the init's return value to the module variable used by attachment). Assignment TARGETS now also receive the module-var transform, so sys.modules[__name__] = ... inside the wrapper body indexes the module's real name rather than the bundle's __name__. Covered by the sys_modules_self_replacement fixture, whose execution snapshot prints the replacement's attribute. Thanks!
| let callee_is_find_spec = match &*call.func { | ||
| Expr::Attribute(attribute) => attribute.attr.as_str() == "find_spec", | ||
| Expr::Name(name) => name.id.as_str() == "find_spec", | ||
| _ => false, | ||
| }; | ||
| if callee_is_find_spec { |
There was a problem hiding this comment.
When a pure dependency calls importlib.reload() on one of its bundled submodules, this detector permits the package because it blocks find_spec and unresolved import calls but not reload. The generated submodule namespace normally has neither a matching sys.modules entry nor an import spec, so the preserved reload raises ImportError even though reloading the installed module succeeds; recognize qualified and imported aliases of reload, keep those packages external (or provide full import-module semantics), and add an isolated execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5af0e11. importlib.reload consumers now stay external: the qualified form (importlib.reload(...)) and from importlib import reload aliases are treated as undiscoverable import callables, so any call blocks bundling of the containing package while unrelated x.reload() methods (common and harmless) remain unaffected by matching only the importlib-qualified/base forms. Covered by the xfail_bundle_third_party_reload fixture (config = importlib.reload(config)), whose requirements snapshot retains reload-pkg. Thanks!
- pkg_resources.working_set / Environment attribute access counts as global distribution enumeration - class-scope bindings inside conditional statements neither rebind nor shadow (Python may never execute them); outer alias resolution is conservatively retained - direct-URL requirements meeting version constraints emit an explicit warning instead of silently dropping the constraint (PEP 508 cannot express both on one line) - name-inferred import roots are verified against the filesystem; a bare dist-info whose project name does not match any module beside it (beautifulsoup4/bs4) marks the ownership index incomplete - relative import_module package anchors resolve verbatim like CPython's _resolve_name: a plain-module anchor produces the non-package target name, the call stays preserved, and the runtime ImportError survives - registered wrapper inits return the final sys.modules entry so deliberate self-replacement propagates to importers (assignment targets now also receive the module-var transform) - importlib.reload consumers stay external: bundled namespaces carry no reloadable spec (qualified form and from-imported aliases)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5af0e11a2f
ℹ️ 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".
| let no_globals = FxIndexSet::default(); | ||
| crate::visitors::LocalVarCollector::new(&mut shadows, &no_globals) | ||
| .ignore_import_bindings() | ||
| .collect_from_stmts(stmts); |
There was a problem hiding this comment.
When a function declares an imported metadata alias global and assigns it later—for example, global md; result = md.version("provider"); md = replacement—passing an empty global set makes the later assignment shadow md for the entire scanner scope. Python instead resolves the query through the module-level alias before that assignment, so the provider query is missed and a metadata-only dependency is omitted from requirements.txt, causing isolated deployments to fail. Collect and pass the scope's global declarations (and handle nonlocal similarly), then cover the query-before-assignment flow with requirements and execution snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5b449d4. Shadow collection now gathers each scope's global declarations first (nested defs excluded) and passes them to the binding collector, so a global md; ...; md = replacement assignment binds the MODULE scope and never shadows — the query resolves through the module-level alias and is recorded. Covered by the new test_queried_distribution_scope_refinements unit case and the xfail_bundle_third_party_metadata_global_alias fixture whose requirements snapshot retains provider. Thanks!
| if QUERY_FUNCTIONS.contains(&path.as_str()) { | ||
| // `require` is variadic; collecting every positional literal is | ||
| // safe for the other APIs too | ||
| for argument in &call.arguments.args { | ||
| if let Expr::StringLiteral(literal) = argument { | ||
| self.requirements.push(literal.value.to_str().to_owned()); | ||
| } |
There was a problem hiding this comment.
When bundled code calls a recognized metadata API with a variable, such as NAME = "provider"; import provider; version(NAME), this branch recognizes the callee but records nothing because only string-literal arguments are collected. The separately imported provider is consequently bundled and removed from requirements, while the preserved runtime query still needs its dist-info and raises PackageNotFoundError in an isolated deployment. Resolve straightforward constants or conservatively keep candidate distributions external whenever a recognized metadata query has an unresolved name, and add requirements and execution snapshots for this flow.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5b449d4 — both halves of your suggestion. Query arguments now resolve as literals OR single-assignment string constants through the scope stack (NAME = "provider"; version(NAME) records provider), and any recognized query with an argument that still cannot be resolved (variables, parameters, **kwargs) conservatively sets the enumeration flag so every installed distribution stays observable. Covered by test_queried_distribution_scope_refinements (constant positive + wrapper-function conservative case) and the xfail_bundle_third_party_metadata_variable_query fixture whose requirements snapshot retains provider. Thanks!
| // Module-scope bindings are collected up front: a function body may query | ||
| // through an alias imported after the function definition | ||
| scopes: vec![Scope { | ||
| bindings: collect_scope_bindings(&module.body), | ||
| shadows: FxIndexSet::default(), |
There was a problem hiding this comment.
When a module queries a metadata alias before reusing that name in a later import—for example, import importlib.metadata as md; VERSION = md.version("provider"); import json as md—precollecting all module bindings leaves only the final json binding before any statements are visited. The valid earlier query is therefore missed, so a metadata-only provider can be omitted from requirements and fail in an isolated bundle. Track module bindings in source order while handling deferred function-body lookup separately, and cover the use-before-reimport case with requirements and execution snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5b449d4. The module scope now carries two views: a source-ordered incremental one for module-level expressions (each top-level statement merges its bindings after being visited, so md.version(...) before import json as md resolves through the earlier metadata alias) and the complete deferred view used when the lookup originates inside a function body, preserving call-time semantics for aliases imported after the function definition. Conditional module-level imports (try/except aliasing) still update bindings without shadowing. Covered by test_queried_distribution_scope_refinements (use-before-reimport and deferred-function cases) and the xfail_bundle_third_party_metadata_use_before_reimport fixture whose requirements snapshot retains provider. Thanks!
| if import_type == Some(crate::visitors::ImportType::ImportlibPreserved) { | ||
| // The call is preserved verbatim and executes as a real runtime import: | ||
| // its target is never bundled through this call, but its distribution | ||
| // must reach requirements generation | ||
| debug!("Recording preserved importlib target: {import}"); | ||
| self.preserved_importlib_targets | ||
| .lock() | ||
| .expect("preserved importlib targets lock poisoned") | ||
| .insert(import.to_owned()); | ||
| return Ok(()); |
There was a problem hiding this comment.
When a first-party module is reachable only through a literal call with opaque arguments, such as options = {}; helper = importlib.import_module("helper", **options), discovery classifies the call as ImportlibPreserved and this branch returns after merely recording its name for requirements. First-party imports do not produce installable requirements, so helper.py is neither embedded nor deployed and the otherwise-valid call fails in the generated single-file bundle. Rewrite statically empty unpacking, provide runtime registration for the bundled target, or reject such first-party calls instead of silently omitting their source, and add an execution snapshot for the target-only flow.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5b449d4 — by solving. Bundleable targets of preserved import_module calls are now BUNDLED: discovery queues them (with parent packages), the classifier wraps them, their init registers them in sys.modules (with __spec__ and the failure-cleanup/final-entry semantics), and the processing phase eagerly initializes them so the preserved runtime call resolves the bundled module through sys.modules inside the single-file bundle. Only genuinely external/unresolvable targets still go to requirements. Covered by the importlib_preserved_first_party fixture: helper = importlib.import_module("helper", **options) prints the bundled first-party value in isolated execution. Thanks!
| const ENUMERATION_FUNCTIONS: [&str; 4] = [ | ||
| "importlib.metadata.distributions", | ||
| "importlib.metadata.packages_distributions", | ||
| "importlib_metadata.distributions", | ||
| "importlib_metadata.packages_distributions", |
There was a problem hiding this comment.
When first-party code calls importlib.metadata.entry_points(group="console_scripts") and imports a pure provider whose only entry points are in console_scripts, this allowlist does not mark the query as global enumeration, while index_entry_points deliberately excludes that group from the provider policy. The provider is therefore bundled and its dist-info removed, so the preserved query no longer observes an entry point that existed in the source environment. Recognize entry-point enumeration APIs (including supported backports) and cover a console-entry-point provider with requirements and execution snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 5b449d4. entry_points (importlib.metadata + importlib_metadata) and pkg_resources.iter_entry_points now count as global distribution enumeration: they observe groups — including console_scripts — that provider policy deliberately excludes, so every installed distribution stays external and observable when they are called. Covered by the xfail_bundle_third_party_entry_points_query fixture (entry_points(group="console_scripts") + a provider whose only entry points are script shims), whose requirements snapshot retains provider. Thanks!
…party targets - global declarations no longer shadow module-level metadata aliases in the query scanner - query arguments resolve as literals or single-assignment string constants; unresolvable arguments conservatively keep every installed distribution observable - module scope carries a source-ordered view for module-level expressions plus the complete deferred view for function bodies (call-time resolution) - first-party or bundleable targets of preserved import_module calls (opaque arguments) are bundled as wrapper modules, registered in sys.modules, and eagerly initialized so the preserved runtime call resolves them inside the single-file bundle - entry_points queries (and iter_entry_points) count as distribution enumeration: they observe groups provider policy deliberately ignores
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b449d4c78
ℹ️ 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".
| if bundler.resolver.is_preserved_importlib_target(module_name) { | ||
| log::debug!("Eagerly initializing preserved importlib target module '{module_name}'"); | ||
| all_inlined_stmts.push( | ||
| crate::ast_builder::module_wrapper::create_wrapper_module_init_call(&module_var), | ||
| ); |
There was a problem hiding this comment.
When a preserved import_module call is inside an uncalled function or untaken branch—or merely follows observable entry code—this unconditional initialization executes the target during bundle startup. A target that prints, mutates state, or raises therefore does so before or without the original import call, changing Python's lazy import semantics. Register or initialize the module at the preserved call site instead, and cover execution ordering and an untaken call with snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 1b0a717 — by solving at the call site. The eager startup initialization is gone: a preserved call now rewrites to (_cribo_init_xxx(<target>), importlib.import_module(...))[1], so the guarded (idempotent) init registers the bundled target in sys.modules exactly when the original call executes — uncalled functions and untaken branches never run the target module. Covered by the importlib_preserved_lazy fixture: its execution snapshot shows "before any import" printed BEFORE the target's import-time side effect, and a never-called function's target (untouched_helper, also side-effectful) produces no output at all. Thanks!
| if call_time && let Some((deferred_bindings, _)) = &scope.deferred { | ||
| return deferred_bindings.get(name); | ||
| } |
There was a problem hiding this comment.
Fresh evidence beyond the addressed module-order case is import importlib.metadata as md; def read(): return md.version("provider"); read(); import json as md: the function executes while the metadata alias is still active, but this always resolves module names through the final deferred binding (json). The query is missed, so an imported provider can be bundled and lose its dist-info even though the preserved runtime query needs it; conservatively account for functions being called before later module rebindings and add a requirements/execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 1b0a717. Function-body lookups against the module scope now consider BOTH views: the source-order bindings at the definition point AND the complete deferred (call-time) view — every alias the name plausibly held applies, since the function may run before or after later rebindings (over-collection is the safe direction). The same union applies to constant resolution. Covered by the new test_queried_distribution_scope_refinements case (query recorded despite a later import json as md) and the xfail_bundle_third_party_metadata_called_before_rebinding fixture whose requirements snapshot retains provider. Thanks!
| // Constants are checked before shadow blocking: a constant | ||
| // assignment is itself a non-import binding | ||
| if let Some(value) = scope.constants.get(name) { | ||
| return Some(value.clone()); |
There was a problem hiding this comment.
Fresh evidence beyond the addressed variable-query case is def read(name): result = version(name); name = "provider"; return result: the body-wide constant collector records the later assignment, and this constants-first lookup overrides the parameter shadow at the earlier call. Invoking read("other-provider") is therefore recorded as a query for provider, allowing other-provider to be bundled and its metadata removed before the real query runs; track function constants in execution order or reject constants that collide with parameters, with a requirements/execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 1b0a717 — via your "reject constants that collide with parameters" option. Function-scope constants are filtered against the parameter set, so a later name = "provider" assignment cannot override the parameter shadow: the earlier version(name) call is unresolvable and conservatively flags every installed distribution as observable, instead of misattributing the caller-supplied value. Covered by the new unit case (empty requirements + enumeration flag for exactly this shape). Thanks!
| if !self.classify_import(module_name).should_bundle() { | ||
| return None; | ||
| } | ||
| let virtualenv_dirs = self.get_virtualenv_site_packages_search_directories(None); | ||
| self.locate_in_directories(module_name, &virtualenv_dirs) |
There was a problem hiding this comment.
When the entry search path contains a plain pkg.py while the selected environment contains a package pkg/sub.py, Python resolves the local pkg first and import pkg.sub raises because that parent is not a package. This fallback nevertheless locates and bundles the site-packages child after normal resolution fails, potentially turning a caught import failure into a successful import and mixing the local parent with an unrelated distribution. Stop the fallback when an earlier search root resolves any parent component as a non-package, and add an execution snapshot for the shadowed-package layout.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 1b0a717. The site-packages bundling fallback now respects Python's parent-first resolution: before falling back, every parent component of the dotted name is probed against the NORMAL search roots, and any hit (package or plain module alike — the child must come from that parent or fail) stops the fallback, so a caught import failure stays a failure and local parents never mix with unrelated distributions. Covered by test_bundle_third_party_respects_local_non_package_parent and the bundle_third_party_local_non_package_parent fixture, whose execution snapshot shows the preserved ImportError alongside the local module winning. Thanks!
…ions - preserved import_module calls initialize their bundled target AT THE CALL SITE via an idempotent init in a tuple expression, preserving Python's lazy import semantics (uncalled functions and untaken branches never execute the target) - function-body lookups against the module scope consider BOTH the source-order view and the deferred (call-time) view: functions called before later module rebindings still record their queries - constants colliding with parameters stay plain shadows, so an earlier call with a caller-supplied value flags conservatively instead of being misattributed - the site-packages bundling fallback respects Python's parent-first resolution: a dotted name whose parent component resolves in a normal search root is never satisfied from site-packages
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b0a7176f7
ℹ️ 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".
| vec![], | ||
| ); | ||
| return Some(expressions::subscript( | ||
| expressions::tuple(vec![init_expr, Expr::Call(call.clone())]), |
There was a problem hiding this comment.
For a preserved call such as importlib.import_module("helper", **options()), Python evaluates options() and validates the resulting arguments before importing anything, but this tuple executes the bundled target's initializer first. If argument evaluation raises or the mapping supplies an invalid/double-bound argument, the original program never executes helper, whereas the bundle executes all of its side effects before raising; even successful calls reverse the observable argument/import order. Delay initialization until after argument evaluation and binding, and add ordering and exception snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| let init_expr = expressions::call( | ||
| expressions::name(init_func_name, ExprContext::Load), | ||
| vec![expressions::name(&module_var, ExprContext::Load)], | ||
| vec![], | ||
| ); |
There was a problem hiding this comment.
When an opaque preserved call targets a dotted module, such as importlib.import_module("pkg.sub", **options), this invokes only pkg.sub's initializer. The discovery path queues pkg, but child initializers explicitly do not initialize their parents, so pkg.__init__ never runs at this call site even though Python always initializes it before pkg.sub; package state, side effects, and attributes can therefore be missing. Use the existing parent-aware initialization path here and add a dotted preserved-import execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| (FxIndexSet::default(), IndexMap::new()) | ||
| }; | ||
| if let Some(scope) = self.scopes.last_mut() { | ||
| scope.bindings.extend(stmt_bindings); |
There was a problem hiding this comment.
Fresh evidence after the conditional-class binding fix is a conditional import: with an enclosing metadata alias followed by if False: import json as md and then md.version("provider"), collect_scope_bindings finds the nested import and this unconditional extend replaces the valid outer alias. The scanner consequently misses the real metadata query and can bundle the provider without its dist-info. Conditional import bindings must be unioned with the prior candidate rather than treated as definitely executed, with module and class execution/requirements snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| if call_time && let Some((deferred_bindings, _)) = &scope.deferred { | ||
| if let Some(target) = deferred_bindings.get(name) { | ||
| candidates.push(target.clone()); | ||
| } | ||
| if let Some(target) = scope.bindings.get(name) | ||
| && !candidates.contains(target) | ||
| { | ||
| candidates.push(target.clone()); | ||
| } | ||
| return candidates; |
There was a problem hiding this comment.
Fresh evidence beyond the called-before-rebinding fix is a function defined while md is json, followed by import importlib.metadata as md; read(); import json as md: the function's scan sees only the source-order binding at definition and the final deferred binding, both json, while its actual call uses the intermediate metadata alias. The provider query is therefore missed and its metadata can be removed. Preserve every plausible module binding between definition and call, or conservatively retain distributions when intermediate execution cannot be determined, and add an execution/requirements snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| crate::visitors::LocalVarCollector::new(&mut shadows, &globals) | ||
| .ignore_import_bindings() | ||
| .collect_from_stmts(stmts); |
There was a problem hiding this comment.
Fresh evidence after the global-declaration fix is nonlocal: for an outer function that imports version and an inner function declaring nonlocal version before calling version("provider"), LocalVarCollector adds the declaration to the inner shadow set and this scanner stops before consulting the outer binding. Python resolves that name to the enclosing import, so the query is real and omitting it can remove the provider's metadata. Exclude nonlocal declarations from local shadows and resolve them through the nearest enclosing function scope, with requirements and execution snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| if alias.name.as_str() == "*" { | ||
| continue; |
There was a problem hiding this comment.
With from importlib.metadata import * followed by version("provider"), Python binds the metadata query API, but this collector discards the wildcard and cannot resolve the call. If the same program imports a pure provider, Cribo can bundle it and remove its dist-info even though the preserved query still requires that metadata at runtime. Expand known metadata __all__ bindings or conservatively mark wildcard imports from supported metadata modules as global metadata observation, with requirements and execution snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| crate::visitors::LocalVarCollector::new(&mut parameter_names, &no_globals) | ||
| .ignore_import_bindings() | ||
| .collect_from_stmts(&func.body); |
There was a problem hiding this comment.
When a function references an outer import before a later local import, as in result = importlib.import_module("helper"); import json as importlib, Python treats importlib as local throughout the function and the first statement raises UnboundLocalError. This pre-pass deliberately ignores import bindings, so discovery, graphing, and transformation instead resolve the outer importlib, potentially bundle and rewrite helper, and remove the exception. Include function-local import targets in the body-wide shadow set while still handling their source order for calls after the import, with an execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| let value_is_import_callable = match value { | ||
| Expr::Call(call) => { | ||
| // A wrapper receiving an import callable (functools.partial and | ||
| // friends) yields a callable that still performs dynamic imports | ||
| call.arguments | ||
| .args | ||
| .iter() | ||
| .any(|argument| is_import_callable(argument, import_aliases, assigned_aliases)) | ||
| || call.arguments.keywords.iter().any(|keyword| { | ||
| is_import_callable(&keyword.value, import_aliases, assigned_aliases) | ||
| }) | ||
| } | ||
| _ => is_import_callable(value, import_aliases, assigned_aliases), |
There was a problem hiding this comment.
Fresh evidence beyond the assigned/higher-order alias handling is an alias inside a function defined before its global source, such as def run(name): load = late; return load(name) followed by late = importlib.import_module and then run(...). Python resolves late when the function executes, but the one-pass pre-collector visits load = late before late has entered assigned_aliases, so calls through load are not recognized as dynamic imports. A third-party package can then be bundled without the selected submodule. Iterate alias propagation to a fixed point and add a deferred-alias execution snapshot.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
With import importlib.metadata as md; query = md.version; query("provider"), the assignment creates a callable alias that performs the same runtime metadata lookup, but the binding collector records only import statements and later treats query as an unrelated shadow. The provider can consequently be bundled and lose its dist-info even though the preserved alias call raises in an isolated deployment. Propagate recognized metadata callables through straightforward assignments, or conservatively retain distributions when such callables escape, and add requirements/execution snapshots.
AGENTS.md reference: AGENTS.md:L245-L250
Useful? React with 👍 / 👎.
| let record_file = dist_info_dir.join("RECORD"); | ||
| if let Some(record) = Self::read_metadata_sidecar(&record_file, incomplete) { |
There was a problem hiding this comment.
Fresh evidence after the missing-RECORD ownership fix is a conventionally named pure package whose distribution also installs a sibling native module, such as frontend/ plus _backend.so. When RECORD is absent, name inference can claim frontend, but this branch never learns that the same distribution ships _backend.so; scanning only frontend/ then marks it bundleable, removes the owning distribution requirement, and cannot ship the sibling binary. Treat absent installed-file listings as insufficient to prove distribution-wide purity, or inspect sibling ownership conservatively, with a targeted error-condition test.
AGENTS.md reference: AGENTS.md:L204-L208
Useful? React with 👍 / 👎.
Summary
Adds an opt-in mode that bundles third-party (site-packages) dependencies into the output, similar to how JavaScript bundlers like esbuild handle
node_modules. Since the Python ecosystem is heavy on native extensions, packages that ship any native artifacts (.so/.pyd) are automatically detected and kept external — the automatic equivalent of esbuild'sexternaloption — and are still emitted intorequirements.txt.Enable via
--bundle-third-party,bundle-third-party = trueincribo.toml, orCRIBO_BUNDLE_THIRD_PARTY=1. Default behavior is unchanged.How it works
resolver.rs): the site-packages branch ofclassify_import_uncachedpreviously hard-codedallow_bundle = false. It now computesallow_bundle = bundle_third_party && !package_has_native_extensions(...).package_has_native_extensionsrecursively scans the module's top-level package directory for.so/.pydfiles (cached per package). A single native artifact anywhere keeps the whole distribution external, so its compiled submodules keep importing correctly at runtime.resolver.rs):resolve_module_path_with_contextand relative-import resolution gained a site-packages fallback, gated on the classification policy so external modules never leak bundle paths into the module cache.orchestrator.rs): under the new mode, imports whose source is inlined are skipped, sorequirements.txtlists only what actually stayed external. Default-mode requirements output is byte-identical.known_third_partyentries always stay external, even when pure Python.Testing
known_third_partyoverride; mode disabled unchanged)cargo clippy --workspace --all-targetsclean;cargo nextest run --workspace: 211/212 pass — the single failure (test_directory_entry_empty_fails) is pre-existing on unmodifiedmain(snapshot path filters don't normalize a/local/home/...workspace prefix) and unrelated to this changeDocs
docs/resolution.md: updated bundle-disposition rules and added a "Third-Party Bundling (Opt-In)" section.Summary by CodeRabbit
New Features
--bundle-third-partycommand-line flag.requirements.txt.requirements.txt.Bug Fixes