Run @{...}@ composition through native pydantic-handlebars#1952
Run @{...}@ composition through native pydantic-handlebars#1952dmontagu wants to merge 10 commits into
Conversation
![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 6 files
Confidence score: 3/5
- There is a concrete medium-risk issue in
logfire/variables/composition.py: the current JSON-null check is brittle and may mis-handle invalid or non-string inputs, which can lead to incorrect parsing behavior. - Given the issue’s 6/10 severity and high confidence (9/10), this introduces some regression risk unless the check is tightened to an exact, trimmed
nullequality with a type guard. - Pay close attention to
logfire/variables/composition.py- null-handling logic should be made strict and type-safe to avoid misclassification of inputs.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Replaces the regex-and-translate workaround in reference_syntax.py and
composition.py with calls into pydantic-handlebars >= 0.2.0's native
configurable-delimiter API.
## What changed
- pyproject.toml: bump `pydantic-handlebars` to `>=0.2.0` (in both the
`[variables]` extra and the dev group) and exempt it from the
`exclude-newer` filter alongside the other Pydantic packages.
- `_handlebars.get_environment()` returns a cached
`HandlebarsEnvironment(open_delim='@{', close_delim='}@')` for the
composition pass. `extract_composition_dependencies()` wraps
`pydantic_handlebars.extract_dependencies` with the same delimiters.
- `reference_syntax.render_once()` shrinks to a one-line call into that
environment. The sentinel-protect-then-regex-translate code path is
gone; `{{...}}` runtime placeholders survive because they're plain
content under the configured delimiters.
- `composition.find_references()` / `_collect_ref_names()` now route
through `extract_composition_dependencies` for AST-correct detection,
then a textual position scan supplies first-occurrence ordering.
## What you can now write
`@{...}@` accepts the full Handlebars syntax — block helpers with
dotted-path headers, `{{#each}}` parent-context references (`../`),
helper subexpressions, the lot. Previously the regex translator only
covered top-level identifiers in block headers, so e.g.
`@{#if user.active}@` silently dropped its condition. New regression
tests cover these in `TestExpandReferencesNativeHandlebarsSyntax` and
`TestFindReferencesNativeHandlebarsSyntax`.
The unresolved-dotted-reference protection in `_render_value` is kept
for behaviour compat (literal `@{name.field}@` is retained in the
output when `name` can't be resolved). That's the
"misrender retention" parity question — addressed separately.
dmontagu
force-pushed
the
feature/variable-composition-native-handlebars
branch
from
98ab03a to
d177c30
Compare
- Use exact, trimmed equality against 'null' in expand_references rather than a case-insensitive startswith match. JSON null is always lowercase, and 'nullify' shouldn't have matched. - Add a test that exercises the 'dotted ref whose base is resolved' branch in _protect_unresolved_dotted_refs, which the regex narrowing in this PR left uncovered.
…le-composition-native-handlebars
…tic/logfire into feature/variable-composition-native-handlebars
| @@ -45,6 +72,22 @@ def get_handlebars_renderer() -> tuple[type[str], Callable[..., str]]: | |||
There was a problem hiding this comment.
do we need this dep check over and over
| # parse strings that actually contain composition syntax. Real reference | ||
| # extraction goes through `pydantic_handlebars.extract_dependencies` so block | ||
| # helpers, dotted paths, and subexpressions are all handled AST-correctly. | ||
| _HAS_REFERENCE = re.compile(r'(?<!\\)@\{') |
There was a problem hiding this comment.
This gate is coupled to the current pydantic-handlebars escaping behavior, where any backslash immediately before @{ escapes the tag. If pydantic-handlebars is changed to match Handlebars.js double-escape behavior, this regex becomes incorrect: \\@{x}@ should contain a real reference with a literal leading backslash, but (?<!\\)@\{ would still return false because the @ is immediately preceded by \. So either this should be explicitly treated as a temporary approximation of the current renderer behavior, or the gate should be parity-aware / delegated to the parser so it keeps matching the renderer if escaping is fixed upstream.
There was a problem hiding this comment.
here's the problematic behaviour in pydantic-handlebars:
from pydantic_handlebars import render
context = {'x': 'X'}
assert render(r'{{x}}', context) == 'X'
assert render(r'\{{x}}', context) == r'{{x}}'
assert render(r'\\{{x}}', context) == r'\{{x}}'
assert render(r'\\\{{x}}', context) == r'\\{{x}}'There was a problem hiding this comment.
in other words, you can't currently 'double escape', i.e. have a literal backslash right before a template field, regardless of delimiter.
| are plain content under the configured delimiters. The escape | ||
| sequence `\@{` produces a literal `@{` in the output. | ||
| """ | ||
| return get_environment().render(template, context) |
There was a problem hiding this comment.
Since this path is now native pydantic-handlebars, it may be worth caching compiled templates here rather than only caching the environment. HandlebarsEnvironment.render() calls compile(source) on every call, and compile() reparses the source each time. For stable managed-variable values, a small bounded cache around get_environment().compile(template) would avoid repeated parsing when the same composed value is resolved multiple times, e.g. @lru_cache(maxsize=1024) returning a CompiledTemplate and then calling .render(context). The same idea may also apply separately to dependency extraction / {{...}} template rendering, but this wrapper is the easiest place to avoid repeated parse work on the composition render path.
Six items Alex flagged as important-but-not-blocking on the original 1951/1952 stack, folded in before #1954 merges: ## Docs - `composition.py` module docstring + `expand_references` docstring no longer claim block helpers are restricted to top-level identifiers — the native handlebars path accepts `@{#if user.active}@` and helper sub-expression headers. - `docs/.../templates-and-composition.md` Control Flow section rewritten to match: drops the "must be top-level" caveat and adds rows for dotted-path conditions and `../` parent-scope access from inside a block. ## Caching - New `compile_composition_template(source)` in `_handlebars` wraps the parse step in an `lru_cache(maxsize=1024)` (#1952 r3289095058). `reference_syntax.render_once` now compiles once per distinct source and reuses the compiled program across resolutions — managed-variable values are typically stable, so the hit rate should be high. - `extract_composition_dependencies` no longer re-runs the `try: from pydantic_handlebars import …` block on every call — the import is moved into a `@cache`d helper that returns the function object directly. Matches the pattern of the existing `_get_template_compatibility_checker` (#1952 r3288194502). ## Warning text - `_composition_failure` renamed to `_fallback_to_default` and takes a `failure_stage` argument (`'composition'` or `'template rendering'`) so the `RuntimeWarning` text reflects the actual failed step. A pydantic-handlebars parse error during `{{...}}` rendering used to surface as `"composition failed"` even though composition succeeded (codex finding). - Updated `test_remote_render_error_records_exception` accordingly. ## Test naming - `test_override_render_failure_falls_back` renamed its locals: `bad_override` → `templated_config` (it's a *valid* template), and introduced `invalid_inputs` for the inputs that actually fail the pattern constraint. Alex r3289312130. ## Escape-detection coupling - Added a comment on the `_HAS_REFERENCE` regex flagging that the lookbehind encodes pydantic-handlebars' current escape semantics ("any preceding `\` escapes") and noting it'll need to count preceding backslashes if pydantic-handlebars adopts Handlebars.js's odd-vs-even-backslash spec behaviour (#1952 r3289062247).
There was a problem hiding this comment.
1 issue found across 14 files (changes from recent commits).
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/_handlebars.py">
<violation number="1" location="logfire/variables/_handlebars.py:100">
P2: Avoid caching a mutable `set` return value here; mutating one call result can corrupt future cached dependency results for the same template.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return _pydantic_handlebars().SafeString | ||
|
|
||
|
|
||
| @lru_cache(maxsize=1024) |
There was a problem hiding this comment.
P2: Avoid caching a mutable set return value here; mutating one call result can corrupt future cached dependency results for the same template.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At logfire/variables/_handlebars.py, line 100:
<comment>Avoid caching a mutable `set` return value here; mutating one call result can corrupt future cached dependency results for the same template.</comment>
<file context>
@@ -51,55 +72,65 @@ def get_environment() -> HandlebarsEnvironment:
- raise _dependency_error() from exc
- raise
- return _check_template_compatibility
+@lru_cache(maxsize=1024)
+def compile_composition_template(source: str) -> CompiledTemplate:
+ """Return a cached `CompiledTemplate` for *source* under composition delimiters.
</file context>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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/_handlebars.py">
<violation number="1" location="logfire/variables/_handlebars.py:100">
P2: Avoid caching a mutable `set` return value here; mutating one call result can corrupt future cached dependency results for the same template.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Closing in favor of #1954 |
3 participants
First in a series implementing the SDK follow-ups from #1950, stacked on #1951.
Replaces the regex-and-translate workaround in
reference_syntax.pyandcomposition.pywith calls intopydantic-handlebars >= 0.2.0's native configurable-delimiter API.What this enables
@{...}@accepts the full Handlebars syntax now:@{#if user.active}@premium@{else}@free@{/if}@. The old regex translator silently dropped the condition becauseuser.activeisn't an identifier.{{#each}}parent-context references with../.@{lookup obj key}@).This closes the syntax-parity item Alex flagged on
templates-and-composition.md:186of #1731.What changed
pyproject.toml: bumpspydantic-handlebarsto>=0.2.0in the[variables]extra and dev group; exempts it from theexclude-newerfilter alongside the other Pydantic packages souv lockpicks up the just-released version._handlebars.py: addsget_environment()(cachedHandlebarsEnvironment(open_delim='@{', close_delim='}@')) andextract_composition_dependencies(template)wrappingpydantic_handlebars.extract_dependencieswith the same delimiters.reference_syntax.py:render_once()shrinks to a one-line call into that environment. The sentinel-protect-then-regex-translate code path is gone.{{...}}runtime placeholders survive untouched because they're plain content under the configured delimiters.composition.py:find_references()/_collect_ref_names()go throughextract_composition_dependenciesfor AST-correct detection (block helpers, dotted paths, subexpressions, helper-name exclusion). A textual word-boundary scan supplies first-occurrence ordering so the publicfind_referencessignature stays alist[str].The unresolved-dotted-reference protection in
_render_valueis kept for behaviour compat (literal@{name.field}@is retained whennamecan't be resolved). That's the misrender-retention parity question — a separate PR in this series.Tests
TestExpandReferencesNativeHandlebarsSyntaxcovers@{#if user.active}@,@{#each list}@@{this}@@{/each}@,../parent refs, and@{lookup obj key}@.TestFindReferencesNativeHandlebarsSyntaxchecks the AST-aware dependency extraction (helper names excluded, scope-shifting respected).Follow-ups in #1950
Next in the sequence: push-time validation (D + E), then render-time strict mode (C), then misrender-retention parity (F), then internal cleanups (G + H).