Address remaining #1954 review threads#1964
Merged
dmontagu merged 4 commits intoMay 25, 2026
Merged
Conversation
Three unresolved threads on #1954, all addressed: ## find_references no longer claims source-order (Alex r3288620699) `_order_by_first_position` used a plain word-boundary regex on the source, so a name that appeared in literal text before any `@{...}@` syntax got ordered first. Repro: find_references('"bar: @{foo}@ @{bar}@"') → ['bar', 'foo'] Getting accurate source-order means walking the AST (the parser is the only thing that knows what's syntax vs. content). Per Alex's suggestion, dropping the ordering requirement entirely: - `find_references` and `_collect_ref_names` return `set[str]`. - `_order_by_first_position` is gone. - Tests that previously asserted list-order use `==` against a set or `{ref.name for ref in composed}`. Callers in `abstract.py` / `template_validation.py` use the result via `.update()` or iteration, so the API switch from `list` to `set` is observable but not breaking inside the SDK. ## ValidationReport.format() no longer prints "Valid" with errors (cubic r3287513617) The "=== Valid (N variables) ===" section was gated on `valid_count > 0` alone, so a report with reference / template-field errors could print BOTH a green "Valid (N)" header AND the red error sections in the same output. Added `and self.is_valid` to the gate so the "Valid" section only renders for fully-clean reports. ## Callable defaults aren't re-invoked on the fallback path (cubic r3287513610) When the code-default tier supplied the value (e.g. provider returned nothing) AND composition / rendering / deserialization then failed, the callable was invoked twice in one `get()`: once via `_get_serialized_default` to feed composition, then again via `_fallback_to_default` / `_resolve_code_default` / the deserialize-error path. Doubling side effects and risking inconsistent values across calls. Introduced `_DEFAULT_CACHE: ContextVar[dict[int, T] | None]` and a new `Variable._get_default_cached(...)`. `_resolve` sets up an empty cache at the top of each `get()` and resets on exit; every fallback path now goes through `_get_default_cached`. The first invocation populates the cache; subsequent invocations return the cached value. Outside a `_resolve` call the cache is `None` and the helper is a pass-through. New regression test `test_callable_default_invoked_once_on_composition_failure` forces the provider to be empty + the callable default to supply the value + composition to fail (via monkeypatched `expand_references`), then asserts the callable was invoked exactly once. 512 variable / composition / template / push / validation tests pass. pyright + ruff format + check clean.
Contributor
There was a problem hiding this comment.
1 issue found across 4 files
Confidence score: 3/5
- There is a concrete medium-risk issue in
logfire/variables/composition.py:_collect_ref_namesusesset[str], which makes composition resolution order nondeterministic. - This can cause unstable
composed_fromordering and inconsistent first-reported composition errors, creating user-visible variability and potential test/debugging churn. - Given severity 6/10 with high confidence (8/10), this is a real regression risk but appears scoped enough to address without broad destabilization.
- Pay close attention to
logfire/variables/composition.py- nondeterministic set iteration can change resolution and error-reporting order.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Cubic r3295934987: switching to `set[str]` made composition resolution order depend on set-iteration order. Two real impacts: - When several refs fail to resolve, *which one* surfaces first in the warning / `composed_from` tree depended on hash iteration. Bug reports and reproductions become flaky. - `composed_from` chain order changed between runs, breaking stable test snapshots and any user code that walks the tree positionally. Switched `find_references` back to `list[str]` — sorted alphabetically. Same property for `_collect_ref_names` callers: `expand_references` now does `sorted(_collect_ref_names(decoded))` so the for-each loop that resolves refs iterates deterministically. Sorting alphabetically isn't source order (which would need an AST walker the parser doesn't expose today), but it IS deterministic — that's what cubic actually flagged. Tests in `TestFindReferences` and `TestFindReferencesNativeHandlebarsSyntax` updated to assert against sorted lists. 512 tests pass.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
fd00885
into
feature/variable-composition-native-handlebars
14 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1954. Closes the three unresolved threads on that PR.
1. Drop the broken "in order" claim on
find_references(Alex r3288620699)_order_by_first_positionused a plain word-boundary regex on the source, so a name appearing in literal text before any@{...}@syntax got ordered first. Alex's repro:Getting accurate source-order means walking the AST — the parser is the only thing that knows what's syntax vs. content. Per Alex's suggestion, dropped the ordering requirement entirely:
find_referencesand_collect_ref_namesnow returnset[str]._order_by_first_positionremoved.{ref.name for ref in composed}.In-SDK callers all use the result via
.update()or iteration, so the switch is observable but not breaking.2.
ValidationReport.format()no longer prints "Valid" alongside errors (cubic r3287513617)The
=== Valid (N variables) ===section was gated only onvalid_count > 0, so a partial-pass report could print both the green "Valid" header and the red reference-error / template-field-issue sections. Addedand self.is_validto the gate.3. Callable defaults aren't re-invoked on the fallback path (cubic r3287513610)
When the code-default tier supplied the value (provider empty) AND composition / rendering / deserialization then failed, the callable was invoked twice in one
get():_lookup_serialized→_get_serialized_defaultto feed composition._fallback_to_default(or the deserialize-error path, or_resolve_code_default).Introduced
_DEFAULT_CACHE: ContextVar[dict[int, T] | None]andVariable._get_default_cached(...)._resolvesets an empty cache at the top of eachget()and resets on exit; every fallback path now goes through_get_default_cached. First call populates, subsequent calls return cached. Outside_resolvethe cache isNoneand the helper is a passthrough (so external callers see no behaviour change).New regression test
test_callable_default_invoked_once_on_composition_failureforces provider-empty + callable-default + composition-failure (via monkeypatchedexpand_references) and asserts the callable was invoked exactly once.Verification
512 variable / composition / template / push / validation tests pass. pyright + ruff format + check clean.