Repository navigation
fix(frontend): match jinja2 scoping when extracting template variables [agent] - #3767
Open
Thanatos9404 wants to merge 1 commit into
Open
Thanatos9404 wants to merge 1 commit into
Thanatos9404 wants to merge 1 commit into
Conversation
extractJinjaVariables() is meant to report the same names as the
backend's jinja2.meta.find_undeclared_variables(), but it kept a single
flat scope and walked a few nodes in the wrong order:
- variables used only in a for loop's {% else %} branch were dropped
- loop targets, "loop" and names set inside a loop stayed declared
after {% endfor %}, hiding inputs with the same name later on
- {% set x = x | trim %} declared x before reading the value
- keyword argument names (tojson(indent=2)) and Jinja2 globals
(range, dict, ...) were reported as inputs
Give loop bodies, for-else branches and block sets their own scope,
read set values before assigning, and skip keyword argument names and
the default jinja2 globals.
|
Thanks for the PR. Before a reviewer picks it up, a few things from CONTRIBUTION_POLICY.md need fixing. This check re-runs on every push and edit. To fix:
If you think a check is wrong, say so here and a maintainer will override it. |
This branch has not been deployed
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.
Summary
extractJinjaVariables()infrontend/src/utils/jinjaVariables.jsdecides which inputs the prompt workbench, Run Prompt, the eval editors and the agent playground ask for when a prompt is in Jinja mode. Its header says it mirrors the backend'sjinja2.meta.find_undeclared_variables()(futureagi/model_hub/utils/jinja_variables.py), but it keeps one flat scope for the whole template and walks a few nodes in the wrong order, so the two disagree on ordinary templates:dev{% for doc in documents %}{{ doc }}{% else %}Nothing found for {{ query }}{% endfor %}documents,querydocuments{% for message in history %}{{ message.content }}{% endfor %}User: {{ message }}history,messagehistory{% set context = context | trim %}{{ context }}context{% for i in range(n) %}{{ i }}{% endfor %}nn,range{{ payload | tojson(indent=2) }}payloadindent,payloadA missing name gets no input or column mapping in the editor, even though the backend's variable list (and the eval
required_keysbuilt from it) includes it. An extra name such asrangeorindenthas no matching dataset column, so Run Prompt lists it as an invalid variable and will not run (RunPrompt.jsxreturns early whileinValidVariablesis non-empty).What changed, all inside
_walk():For: the iterable is read in the enclosing scope, the body gets its own copy of the scope (loop target,loop, anything set inside), and the{% else %}branch is now walked, in the enclosing scope.Set:{% set x = expr %}readsexprbefore declaringx.{% set x %}...{% endset %}declaresxfirst and gives the body its own scope. Both match what jinja2 reports.KeywordArgs: only the values are walked, soindentintojson(indent=2)is no longer an input.Symbol: names in a defaultjinja2.Environment().globals(cycler,dict,joiner,lipsum,namespace,range) are skipped, asfind_undeclared_variables()does.Linked issues
No issue. This is a small bug fix (+39/-13 in the source file), which CONTRIBUTION_POLICY rule 1 allows straight to a PR. The why is in the summary above.
AI use
AI use: Claude Code (agent, opening this PR for the author) found the mismatch, wrote the fix and the tests, and ran every check pasted below.
Type of change
E2E coverage
E2E: exempt (harness-gap no flow exercises Jinja-mode variable extraction)
node scripts/e2e-coverage.mjsfrome2e/reports no flow touching this file and passes with this marker.Tests
frontend/src/utils/__tests__/jinjaVariables.test.js(new, 13 tests). Each expected list is the output offind_undeclared_variables()on jinja2 3.1.6, the version pinned infutureagi/uv.lock. A small script pulled every template and expected list out of the test file and ran them through jinja2: 15 of 15 assertions match.nulland unparseable templates return[]loopstay local to the body{% else %}are collected{% endfor %}loopare not visible in{% else %}{% for row in row %}reads the iterable in the enclosing scope{% set context = context | trim %}still needscontextsetmakes the name local afterwardssethas its own scoperange,cycler,dict) are not reportedWith the fix:
Same test file with
jinjaVariables.jsreverted todev(9 of 13 fail, the 4 that pass cover behaviour that was already right):Every test file that imports the module, directly or through
src/utils/utils.js(vitest related --run src/utils/jinjaVariables.js):Those 22 files were then run on their own, once with
jinjaVariables.jsfromdevand once with this branch. Both runs fail the same 10 files and the same number of tests:The other 12 pass on their own with and without the change. In the full run every worker was busy (several 30 s timeouts) and the disk on this machine was nearly full around then. The 10 that also fail on
devcome from this setup, not from this change: the checkout was frontend-only (the host-update andws-close-codestests readapi_contracts/andfutureagi/), and the Indian system locale formats numbers and dates differently from what some text matchers expect (for example/104,233 sessions in the Sessions view/).prettier --writeandeslinton both changed files: no changes after formatting, no lint errors.Beyond the unit tests, 30 templates were run through both jinja2 3.1.6 and this function:
devdisagrees on 18 of them, this branch on 2 (listed below).Edge cases and what this does not cover
{% if %}and used after it: jinja2 still reports it, this function does not (unchanged fromdev).dev).[], while the backend falls back to a regex (unchanged).These are less common in prompts and each needs more than a small fix, so they are left out here.
Environment and what was not run
yarn install --frozen-lockfile --ignore-enginesbecausepackage.jsonasks for Node >= 22.18.0../bin/dev): Docker is not available on this machine. The change is limited to a pure function with no side effects, which the tests call directly.Checklist
bin/testpasses locally (backend): not applicable, frontend onlyyarn test:runpasses locally (frontend): see Tests above, 10 files fail the same way ondevin this environmentyarn contracts:checkpasses if API surface changed: no API change