[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 package paths. The default model-inclusion fallback excludes those modules unless explicitly included. Tests cover module-spec filtering, include overrides, built-in and namespace handling, and updated metric and SQLModel snapshot expectations. 🚥 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)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 4/5
- In
tests/test_pydantic_plugin.py, the inline snapshots still reference outdatedcode.linenovalues after new tests shifted file lines, which can cause snapshot mismatches and flaky/failing test expectations even if runtime behavior is unchanged—re-record or update the affected snapshots 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="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
| @@ -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>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
2e628b2 to
0ec2705
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
logfire/integrations/pydantic.py (2)
439-457: 📐 Maintainability & Code Quality | 🔵 TrivialFallback correctly gated on non-user-code classification.
Matches the documented default ("third party modules are not instrumented by the plugin to avoid noise") and preserves
include/excludeoverride precedence.Separately: linked-repo context shows
pydantic/pydantic's docs describeinstrument_pydantic()as logging "all Pydantic models in your project," andpydantic-ai's docs assume broad instrumentation without mentioning this filtering/override. Those external docs may now be more clearly stale as a result of this fix — worth a follow-up doc note in those repos.🤖 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 439 - 457, Update the relevant Pydantic integration documentation to note that default instrumentation excludes third-party/non-user-code modules, while include and exclude patterns can override this behavior. Keep the implementation in _include_model unchanged and limit the documentation follow-up to the affected external usage descriptions.Source: Linked repositories
375-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftShare the non-user-code prefix logic
The non-user-code prefix logic overlaps withlogfire/_internal/stack_info.py, but the sources aren’t identical (stack_info.pyseeds its set from runtime package paths, while this code rebuilds it fromsysconfig/site). Extract the prefix derivation into a shared helper so the two filters stay in sync.🤖 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 375 - 386, Extract the prefix derivation currently implemented by _non_user_code_prefixes into a shared helper, reusing the existing runtime package-path sources from logfire/_internal/stack_info.py rather than maintaining separate sysconfig/site logic. Update both _non_user_code_prefixes and the stack-info filter to consume that helper, preserving their current filtering behavior while keeping the prefix sets synchronized.
🤖 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.
Nitpick comments:
In `@logfire/integrations/pydantic.py`:
- Around line 439-457: Update the relevant Pydantic integration documentation to
note that default instrumentation excludes third-party/non-user-code modules,
while include and exclude patterns can override this behavior. Keep the
implementation in _include_model unchanged and limit the documentation follow-up
to the affected external usage descriptions.
- Around line 375-386: Extract the prefix derivation currently implemented by
_non_user_code_prefixes into a shared helper, reusing the existing runtime
package-path sources from logfire/_internal/stack_info.py rather than
maintaining separate sysconfig/site logic. Update both _non_user_code_prefixes
and the stack-info filter to consume that helper, preserving their current
filtering behavior while keeping the prefix sets synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 272fe268-f669-465f-96cc-6b07d75875e1
📒 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)
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