[codex] Fix Pydantic default third-party filtering#2054
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe Pydantic integration now classifies modules as "non-user code" when they resolve to standard library, site-packages, or Logfire's own package paths, using cached helpers built on importlib.util, site, and sysconfig lookups. The default fallback in the model-inclusion decision (_include_model) changed from always returning True to excluding non-user-code modules unless explicitly included. Corresponding tests were added to verify default exclusion of third-party and built-in modules, override behavior via include patterns, and handling of invalid or namespace module specs. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_pydantic_plugin.py (2)
202-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fake_find_spechere is never exercised.In
_include_model, theincludebranch returns before the non-user-code fallback, so_module_is_non_user_code(and thusfind_spec) isn't called whenincludeis set. The test still validly confirmsincludewins, but the patch is dead weight and the name overstates what's verified. Optional to drop the patch.🤖 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 `@tests/test_pydantic_plugin.py` around lines 202 - 219, The test currently patches importlib.util.find_spec via fake_find_spec, but that path is never reached when include is provided because _include_model returns before the non-user-code fallback. Update test_pydantic_plugin_include_overrides_default_non_user_filter to remove the unnecessary patch (and fake_find_spec setup) or otherwise adjust it so it actually exercises the intended branch; the key behavior to keep asserting is that _new_pydantic_plugin_result returns a non-empty result when include={'third_party_pkg.models::ThirdPartyModel'} is set.
162-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_module_is_non_user_codeislru_cached — clear it to avoid cross-test contamination.Results are cached by module name across tests. It works today only because each test uses distinct fake module names; a future test reusing a name would silently get a stale, differently-patched result. Clear the cache in the helper (or a fixture) to keep tests hermetic.
from logfire.integrations.pydantic import _module_is_non_user_code, _non_user_code_prefixes _module_is_non_user_code.cache_clear() _non_user_code_prefixes.cache_clear()🤖 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 `@tests/test_pydantic_plugin.py` around lines 162 - 180, The test helper _new_pydantic_plugin_result is leaving cached state in _module_is_non_user_code and _non_user_code_prefixes between test runs, which can cause cross-test contamination. Update the helper (or a shared fixture used by it) to clear both caches before calling LogfirePydanticPlugin.new_schema_validator, so each test gets a fresh result regardless of reused module names.
🤖 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 `@logfire/integrations/pydantic.py`:
- Around line 397-422: _module_is_non_user_code() should not let
importlib.util.find_spec() failures abort validation when resolving module
metadata. Broaden the exception handling around the find_spec(module) call to
catch any exception, not just ImportError/ModuleNotFoundError/ValueError, and
return False so the module is treated as user code. Keep the existing behavior
in _module_is_non_user_code and preserve the caching and spec checks after a
successful lookup.
---
Nitpick comments:
In `@tests/test_pydantic_plugin.py`:
- Around line 202-219: The test currently patches importlib.util.find_spec via
fake_find_spec, but that path is never reached when include is provided because
_include_model returns before the non-user-code fallback. Update
test_pydantic_plugin_include_overrides_default_non_user_filter to remove the
unnecessary patch (and fake_find_spec setup) or otherwise adjust it so it
actually exercises the intended branch; the key behavior to keep asserting is
that _new_pydantic_plugin_result returns a non-empty result when
include={'third_party_pkg.models::ThirdPartyModel'} is set.
- Around line 162-180: The test helper _new_pydantic_plugin_result is leaving
cached state in _module_is_non_user_code and _non_user_code_prefixes between
test runs, which can cause cross-test contamination. Update the helper (or a
shared fixture used by it) to clear both caches before calling
LogfirePydanticPlugin.new_schema_validator, so each test gets a fresh result
regardless of reused module names.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42c8ad84-2802-4080-8dc2-a6cac28c04ee
📒 Files selected for processing (2)
logfire/integrations/pydantic.pytests/test_pydantic_plugin.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/logfire(manual)pydantic/pydantic-ai(manual)pydantic/pydantic(auto-detected)
| @lru_cache | ||
| def _module_is_non_user_code(module: str) -> bool: | ||
| if not module: | ||
| return False | ||
|
|
||
| try: | ||
| spec = importlib.util.find_spec(module) | ||
| except (ImportError, ModuleNotFoundError, ValueError): | ||
| return False | ||
|
|
||
| if spec is None: | ||
| return False | ||
|
|
||
| if spec.origin in ('built-in', 'frozen'): | ||
| return True | ||
|
|
||
| module_paths: list[Path] = [] | ||
| if spec.origin and not spec.origin.startswith('<'): | ||
| module_paths.append(Path(spec.origin).resolve()) | ||
| module_paths.extend(Path(path).resolve() for path in spec.submodule_search_locations or ()) | ||
|
|
||
| if not module_paths: | ||
| return False | ||
|
|
||
| prefixes = _non_user_code_prefixes() | ||
| return all(any(_path_has_prefix(path, prefix) for prefix in prefixes) for path in module_paths) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files logfire/integrations/pydantic.py
printf '\n== outline ==\n'
ast-grep outline logfire/integrations/pydantic.py --view expanded || true
printf '\n== relevant symbols ==\n'
rg -n "handle_internal_errors|_include_model|new_schema_validator|_module_is_non_user_code|find_spec" logfire/integrations/pydantic.py
printf '\n== surrounding lines ==\n'
sed -n '340,460p' logfire/integrations/pydantic.pyRepository: pydantic/logfire
Length of output: 7159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== handle_internal_errors in file ==\n'
rg -n "handle_internal_errors" logfire/integrations/pydantic.py || true
printf '\n== wider call chain around new_schema_validator ==\n'
sed -n '290,370p' logfire/integrations/pydantic.py
printf '\n== pydantic integration docs for context ==\n'
sed -n '1,120p' docs/integrations/pydantic.md 2>/dev/null || trueRepository: pydantic/logfire
Length of output: 7213
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import importlib.util
import os
import sys
import tempfile
from pathlib import Path
def probe(exc_name: str):
with tempfile.TemporaryDirectory() as d:
root = Path(d)
pkg = root / "pkg"
pkg.mkdir()
(pkg / "__init__.py").write_text(f"raise {exc_name}('boom')\n")
(pkg / "child.py").write_text("x = 1\n")
sys.path.insert(0, d)
try:
try:
importlib.util.find_spec("pkg.child")
return f"{exc_name}: no exception"
except Exception as e:
return f"{exc_name}: {type(e).__name__}: {e}"
finally:
sys.path.remove(d)
for exc in ["ValueError", "RuntimeError", "ImportError"]:
print(probe(exc))
PY
Length of output: 218
Catch failures from find_spec() here
importlib.util.find_spec() can execute parent package imports, so a broken pkg/__init__.py can raise RuntimeError/other exceptions here and abort model creation. Catch Exception and fall back to treating the module as user code so plugin setup never crashes validation.
🤖 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 `@logfire/integrations/pydantic.py` around lines 397 - 422,
_module_is_non_user_code() should not let importlib.util.find_spec() failures
abort validation when resolving module metadata. Broaden the exception handling
around the find_spec(module) call to catch any exception, not just
ImportError/ModuleNotFoundError/ValueError, and return False so the module is
treated as user code. Keep the existing behavior in _module_is_non_user_code and
preserve the caching and spec checks after a successful lookup.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 3/5
- In
logfire/integrations/pydantic.py, thefind_spec()call for dotted module names can trigger parent-package imports that raise exceptions not covered by the current(ImportError, ModuleNotFoundError, ValueError)handler, which could cause unexpected crashes during plugin detection instead of a graceful fallback — broaden handling around the probe (or isolate parent-import failures) before merging. - In
tests/test_pydantic_plugin.py, shifted test line numbers left inline snapshots with stalecode.linenovalues, which can produce noisy or failing snapshot checks and mask real regressions in later changes — refresh/update the affected snapshots so test expectations match the new layout before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="logfire/integrations/pydantic.py">
<violation number="1" location="logfire/integrations/pydantic.py:404">
P2: The `except` clause here only catches `(ImportError, ModuleNotFoundError, ValueError)`, but `importlib.util.find_spec()` automatically imports parent packages when given a dotted name (e.g., `third_party_pkg.models`). A broken parent `__init__.py` can raise arbitrary exceptions (`RuntimeError`, `SyntaxError`, `OSError`, etc.) that would propagate uncaught and abort model creation. Consider broadening to `except Exception` and falling back to treating the module as user code, so the plugin never crashes validation.</violation>
</file>
<file name="tests/test_pydantic_plugin.py">
<violation number="1" location="tests/test_pydantic_plugin.py:197">
P2: The new test functions added before the existing snapshot-based tests have shifted line numbers in this file, but the inline snapshots still contain the old `code.lineno: 123` values captured before the change. Since the plugin records the physical line number where validation occurs, and the `MyModel(x='a')` calls now reside at different lines (265, 341, etc.), all 23 inline snapshots with `code.lineno` will fail when the test suite runs with inline_snapshot in review mode. Run `pytest --inline-snapshot=update` to refresh the snapshot values, then commit the updated line numbers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| try: | ||
| spec = importlib.util.find_spec(module) | ||
| except (ImportError, ModuleNotFoundError, ValueError): |
There was a problem hiding this comment.
P2: The except clause here only catches (ImportError, ModuleNotFoundError, ValueError), but importlib.util.find_spec() automatically imports parent packages when given a dotted name (e.g., third_party_pkg.models). A broken parent __init__.py can raise arbitrary exceptions (RuntimeError, SyntaxError, OSError, etc.) that would propagate uncaught and abort model creation. Consider broadening to except Exception and falling back to treating the module as user code, so the plugin never crashes validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At logfire/integrations/pydantic.py, line 404:
<comment>The `except` clause here only catches `(ImportError, ModuleNotFoundError, ValueError)`, but `importlib.util.find_spec()` automatically imports parent packages when given a dotted name (e.g., `third_party_pkg.models`). A broken parent `__init__.py` can raise arbitrary exceptions (`RuntimeError`, `SyntaxError`, `OSError`, etc.) that would propagate uncaught and abort model creation. Consider broadening to `except Exception` and falling back to treating the module as user code, so the plugin never crashes validation.</comment>
<file context>
@@ -368,6 +372,56 @@ def new_schema_validator(
+
+ try:
+ spec = importlib.util.find_spec(module)
+ except (ImportError, ModuleNotFoundError, ValueError):
+ return False
+
</file context>
| except (ImportError, ModuleNotFoundError, ValueError): | |
| except Exception: |
| @@ -2,7 +2,9 @@ | |||
|
|
|||
There was a problem hiding this comment.
P2: The new test functions added before the existing snapshot-based tests have shifted line numbers in this file, but the inline snapshots still contain the old code.lineno: 123 values captured before the change. Since the plugin records the physical line number where validation occurs, and the MyModel(x='a') calls now reside at different lines (265, 341, etc.), all 23 inline snapshots with code.lineno will fail when the test suite runs with inline_snapshot in review mode. Run pytest --inline-snapshot=update to refresh the snapshot values, then commit the updated line numbers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_pydantic_plugin.py, line 197:
<comment>The new test functions added before the existing snapshot-based tests have shifted line numbers in this file, but the inline snapshots still contain the old `code.lineno: 123` values captured before the change. Since the plugin records the physical line number where validation occurs, and the `MyModel(x='a')` calls now reside at different lines (265, 341, etc.), all 23 inline snapshots with `code.lineno` will fail when the test suite runs with inline_snapshot in review mode. Run `pytest --inline-snapshot=update` to refresh the snapshot values, then commit the updated line numbers.</comment>
<file context>
@@ -157,6 +159,89 @@ def test_logfire_plugin_include_exclude_models(
+ return SimpleNamespace(origin=__file__, submodule_search_locations=[])
+ return None
+
+ with patch('logfire.integrations.pydantic.importlib.util.find_spec', side_effect=fake_find_spec):
+ assert _new_pydantic_plugin_result('third_party_pkg.models', 'ThirdPartyModel') == (None, None, None)
+ assert _new_pydantic_plugin_result('tests.test_pydantic_plugin', 'MyModel') != (None, None, None)
</file context>
1 participant
Summary
includeis emptyinclude=as an explicit override so third-party modules can still be instrumented intentionallyWhy
Fixes #2053. The current implementation instruments every model outside a tiny hardcoded ignore list when
includeis empty, which contradicts the documented default and can create high-volume noise from third-party validation fallbacks.Validation
git diff --checkinclude=override for an explicit third-party modulepytest tests/test_pydantic_plugin.py -qis blocked in this environment before test collection by an existing OpenTelemetry/importlib deprecation warning promoted to error by the repo's pytest warning policy