Extract variable resolution structure#1947
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if default_result is not None: | ||
| return default_result |
There was a problem hiding this comment.
🟡 Provider exception lost when _resolve_serialized_default returns a result via render_fn path
When render_fn is not None and the provider returns value=None with a non-None exception (e.g., reason='unrecognized_variable'), the _resolve_serialized_default method at line 245-246 returns a result that does not carry the provider's exception from serialized_result.exception. This is because _resolve_serialized_default creates a fresh ResolvedVariable at line 350 without the provider exception, and the code at line 245-246 returns it directly, bypassing line 248-255 which preserves serialized_result.exception.
The test test_get_preserves_provider_exception_when_using_code_default (tests/test_variables.py:1766) demonstrates the intent that provider exceptions should be preserved when falling back to code default. The plain Variable.get() path (render_fn=None) is unaffected since _resolve_serialized_default returns None in that case. But the future TemplateVariable path (render_fn != None) will lose this exception information.
Prompt for agents
In _resolve method (logfire/variables/variable.py), when _resolve_serialized_default returns a non-None result at line 245-246, the provider exception from serialized_result.exception is lost. The _resolve_serialized_default method (lines 334-361) creates a fresh ResolvedVariable at line 350 that doesn't carry the provider's exception.
To fix this, either:
1. Pass serialized_result.exception into _resolve_serialized_default and have it attach the exception to the returned result, OR
2. After getting default_result back (line 245), set default_result.exception = serialized_result.exception before returning it (or merge exceptions if both exist).
The fix should ensure that when a provider returns value=None with an exception (like RuntimeError('missing') for unrecognized variables), that exception is preserved in the final ResolvedVariable even when render_fn processes the code default.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
3 issues found across 2 files
Confidence score: 3/5
- There is concrete behavioral risk in
logfire/variables/variable.py:Variable.get()can passNoneintorender_fn, causing resolve-function defaults to run twice on missing-config lookups, which may produce side effects or inconsistent defaults. - Error-handling paths look fragile in the same file:
_deserializenot catchingTypeErrorcan let validator errors escape and lose label/version metadata, and_resolve_serialized_defaultcan mask a provider exception whenrender_fnreturns a non-Nonevalue. - Given one medium-high issue (6/10, high confidence) plus additional medium issues, this looks mergeable with caution rather than a low-risk change.
- Pay close attention to
logfire/variables/variable.py- default resolution and exception propagation paths may alter runtime behavior and debugging signals.
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/variables/variable.py">
<violation number="1" location="logfire/variables/variable.py:174">
P2: Catch `TypeError` in `_deserialize`. Pydantic v2 lets validator `TypeError`s bubble out, and this path then drops the label/version metadata for the bad value.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| """ | ||
| return None | ||
|
|
||
| def _deserialize(self, serialized_value: str) -> T_co | ValidationError | ValueError: |
There was a problem hiding this comment.
P2: Catch TypeError in _deserialize. Pydantic v2 lets validator TypeErrors bubble out, and this path then drops the label/version metadata for the bad value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At logfire/variables/variable.py, line 174:
<comment>Catch `TypeError` in `_deserialize`. Pydantic v2 lets validator `TypeError`s bubble out, and this path then drops the label/version metadata for the bad value.</comment>
<file context>
@@ -157,11 +163,19 @@ def __init__(
+ """
+ return None
+
+ def _deserialize(self, serialized_value: str) -> T_co | ValidationError | ValueError:
"""Deserialize a JSON string to the variable's type, returning an Exception on failure."""
try:
</file context>
| serialized_default = self._get_serialized_default(targeting_key, attributes) | ||
| if serialized_default is None: | ||
| return None | ||
| if render_fn is None: | ||
| return None |
There was a problem hiding this comment.
P2: Check render_fn before _get_serialized_default(). Plain Variable.get() passes None, so missing-config lookups call resolve-function defaults twice.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At logfire/variables/variable.py, line 343:
<comment>Check `render_fn` before `_get_serialized_default()`. Plain `Variable.get()` passes `None`, so missing-config lookups call resolve-function defaults twice.</comment>
<file context>
@@ -356,6 +321,45 @@ def _get_default(
+ render_fn: Callable[[str], str] | None = None,
+ ) -> ResolvedVariable[T_co] | None:
+ """Resolve the code default through composition/rendering when needed."""
+ serialized_default = self._get_serialized_default(targeting_key, attributes)
+ if serialized_default is None:
+ return None
</file context>
| serialized_default = self._get_serialized_default(targeting_key, attributes) | |
| if serialized_default is None: | |
| return None | |
| if render_fn is None: | |
| return None | |
| if render_fn is None: | |
| return None | |
| serialized_default = self._get_serialized_default(targeting_key, attributes) | |
| if serialized_default is None: | |
| return None |
✅ Addressed in d19c2d7
| return default_result | ||
| # Provider had no value; surface that the code default was used. |
There was a problem hiding this comment.
P2: Provider exception from serialized_result.exception is lost when _resolve_serialized_default returns a non-None result (i.e., when render_fn is provided). The _resolve_serialized_default method creates a fresh ResolvedVariable without propagating the provider's exception. This means if a provider returns value=None with a meaningful exception (e.g., reason='unrecognized_variable'), that diagnostic information is silently dropped in the render_fn code path, whereas the non-render_fn path at lines 248-255 correctly preserves it via exception=serialized_result.exception.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At logfire/variables/variable.py, line 246:
<comment>Provider exception from `serialized_result.exception` is lost when `_resolve_serialized_default` returns a non-None result (i.e., when `render_fn` is provided). The `_resolve_serialized_default` method creates a fresh `ResolvedVariable` without propagating the provider's exception. This means if a provider returns `value=None` with a meaningful exception (e.g., `reason='unrecognized_variable'`), that diagnostic information is silently dropped in the render_fn code path, whereas the non-render_fn path at lines 248-255 correctly preserves it via `exception=serialized_result.exception`.</comment>
<file context>
@@ -278,34 +227,23 @@ def _resolve(
+ render_fn=render_fn,
+ )
+ if default_result is not None:
+ return default_result
# Provider had no value; surface that the code default was used.
return ResolvedVariable(
</file context>
| return default_result | |
| # Provider had no value; surface that the code default was used. | |
| if default_result is not None: | |
| default_result.exception = default_result.exception or serialized_result.exception | |
| return default_result |
✅ Addressed in 1147a16
|
Superseded by #1731 — the structural extract from this branch was merged into the variable-composition branch in b2f4c76, and the architectural follow-up that David was working on (extract a shared lookup helper so composition's |
2 participants
Stacked on #1945.
This extracts the non-template/non-composition variable structure from the larger composition branch:
_BaseVariablewith the existing shared variable resolution implementationVariablea thin subclass whoseget()delegates to_get_result_and_record_span()_record_exception()for variable span exception recording and tests for the CPython traceback edge caseIntentionally excludes template variables, composition expansion, composed metadata, serialized-default composition, and template schema changes.