Skip to content

fix(frontend): match jinja2 scoping when extracting template variables [agent] - #3767

Open
Thanatos9404 wants to merge 1 commit into
future-agi:devfrom
Thanatos9404:fix/jinja-variable-scoping
Open

Thanatos9404 wants to merge 1 commit into
future-agi:devfrom
Thanatos9404:fix/jinja-variable-scoping

Conversation

@Thanatos9404

Copy link
Copy Markdown

Summary

extractJinjaVariables() in frontend/src/utils/jinjaVariables.js decides 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's jinja2.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:

Template Backend (jinja2 3.1.6) Frontend on dev
{% for doc in documents %}{{ doc }}{% else %}Nothing found for {{ query }}{% endfor %} documents, query documents
{% for message in history %}{{ message.content }}{% endfor %}User: {{ message }} history, message history
{% set context = context | trim %}{{ context }} context none
{% for i in range(n) %}{{ i }}{% endfor %} n n, range
{{ payload | tojson(indent=2) }} payload indent, payload

A missing name gets no input or column mapping in the editor, even though the backend's variable list (and the eval required_keys built from it) includes it. An extra name such as range or indent has no matching dataset column, so Run Prompt lists it as an invalid variable and will not run (RunPrompt.jsx returns early while inValidVariables is 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 %} reads expr before declaring x. {% set x %}...{% endset %} declares x first and gives the body its own scope. Both match what jinja2 reports.
  • KeywordArgs: only the values are walked, so indent in tojson(indent=2) is no longer an input.
  • Symbol: names in a default jinja2.Environment().globals (cycler, dict, joiner, lipsum, namespace, range) are skipped, as find_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

  • 🐛 Bug fix (non-breaking change which fixes an issue)

E2E coverage

E2E: exempt (harness-gap no flow exercises Jinja-mode variable extraction)

node scripts/e2e-coverage.mjs from e2e/ 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 of find_undeclared_variables() on jinja2 3.1.6, the version pinned in futureagi/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.

  • plain, dotted and filtered references return sorted root names
  • empty, null and unparseable templates return []
  • for loop target and loop stay local to the body
  • inputs used only in {% else %} are collected
  • the loop target is not declared after {% endfor %}
  • the loop target and loop are not visible in {% else %}
  • names set inside a loop do not leak out
  • {% for row in row %} reads the iterable in the enclosing scope
  • {% set context = context | trim %} still needs context
  • a top level set makes the name local afterwards
  • a block set has its own scope
  • jinja2 globals (range, cycler, dict) are not reported
  • keyword argument names are not reported

With the fix:

 ✓ src/utils/__tests__/jinjaVariables.test.js (13 tests) 12ms
 Test Files  1 passed (1)
      Tests  13 passed (13)

Same test file with jinjaVariables.js reverted to dev (9 of 13 fail, the 4 that pass cover behaviour that was already right):

   ✓ extractJinjaVariables > returns sorted root names of plain, dotted and filtered references 6ms
   ✓ extractJinjaVariables > returns an empty list for empty or unparseable templates 5ms
   ✓ extractJinjaVariables > for loops > treats the loop target and loop as local to the body 1ms
   × extractJinjaVariables > for loops > collects inputs used only in the else branch 10ms
   × extractJinjaVariables > for loops > does not keep the loop target after endfor 2ms
   × extractJinjaVariables > for loops > does not expose the loop target or loop to the else branch 2ms
   × extractJinjaVariables > for loops > does not keep names set inside the body 1ms
   × extractJinjaVariables > for loops > reads the iterable in the enclosing scope 1ms
   × extractJinjaVariables > set > reads the value before assigning the target 2ms
   ✓ extractJinjaVariables > set > treats the target as local after the assignment 1ms
   × extractJinjaVariables > set > gives a block set its own scope 1ms
   × extractJinjaVariables > does not report Jinja2 globals such as range 1ms
   × extractJinjaVariables > does not report keyword argument names 1ms
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 9 ⎯⎯⎯⎯⎯⎯⎯
 Test Files  1 failed (1)
      Tests  9 failed | 4 passed (13)

Every test file that imports the module, directly or through src/utils/utils.js (vitest related --run src/utils/jinjaVariables.js):

 Test Files  22 failed | 377 passed (399)
      Tests  37 failed | 4732 passed | 13 skipped (4782)

Those 22 files were then run on their own, once with jinjaVariables.js from dev and once with this branch. Both runs fail the same 10 files and the same number of tests:

 Test Files  10 failed | 12 passed (22)
      Tests  35 failed | 286 passed (321)

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 dev come from this setup, not from this change: the checkout was frontend-only (the host-update and ws-close-codes tests read api_contracts/ and futureagi/), 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 --write and eslint on 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: dev disagrees on 18 of them, this branch on 2 (listed below).

Edge cases and what this does not cover

  • A name set in only one branch of an {% if %} and used after it: jinja2 still reports it, this function does not (unchanged from dev).
  • Macros: the macro name and its positional parameters are still reported as inputs (unchanged from dev).
  • Templates nunjucks cannot parse still return [], 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

  • Windows 11, Node 22.17.1, vitest 3.2.3. Dependencies were installed with yarn install --frozen-lockfile --ignore-engines because package.json asks for Node >= 22.18.0.
  • The app was not run against the full stack (./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

  • My code follows the style guide
  • I've added tests that prove my fix is effective or that my feature works
  • bin/test passes locally (backend): not applicable, frontend only
  • yarn test:run passes locally (frontend): see Tests above, 10 files fail the same way on dev in this environment
  • yarn contracts:check passes if API surface changed: no API change
  • I've updated the documentation where relevant: no user docs affected
  • No hardcoded secrets, URLs, or PII
  • I've signed the CLA: will sign when the bot asks
  • PR title follows Conventional Commits

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.
@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

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:

  • No linked issue. Anything bigger than a small fix needs an issue a maintainer has labelled accepted before the PR (policy rule 1). Add Closes #N to the body, or open one and wait for the label.

If you think a check is wrong, say so here and a maintainer will override it.

@github-actions github-actions Bot added the needs-admission Admission gate found template/issue/branch/hygiene problems label Nov 7, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend needs-admission Admission gate found template/issue/branch/hygiene problems

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant