Uh oh!
There was an error while loading. Please reload this page.
Fix Admin Settings 500: Jinja set scope does not cross an include - #1322
Open
Paul Lizer (paullizer) wants to merge 1 commit into
Open
Fix Admin Settings 500: Jinja set scope does not cross an include#1322Paul Lizer (paullizer) wants to merge 1 commit into
Paul Lizer (paullizer) wants to merge 1 commit into
Conversation
Visiting Admin Settings raised UndefinedError on analyze_capability and
returned a 500 for every request.
Root cause
Jinja {% set %} scope does not cross an {% include %} boundary. While
Admin Settings was one 14,000 line template that never mattered: a
variable derived near the top was visible everywhere below it. The
information architecture rework split it into 44 per-tab partials, and
a variable derived in one partial is not visible in another.
Three variables were left behind when their consuming card moved:
analyze_capability agents -> actions 500 error
comparison_capability agents -> actions 500 error
enable_dai_debug redis -> cosmos silent
Two failure modes, and the quiet one is worse
Attribute access on a missing name raises and takes the page down.
Loud, and found immediately.
A boolean test on a missing name is silently false, because Jinja's
default Undefined is falsy. Nothing errors. The Cosmos tab's debug
controls and shadow validation diagnostics would never have rendered,
whatever the setting was, and nothing would have reported a problem.
That one was found only by auditing for the pattern that caused the
crash.
Each declaration is a pure derivation from settings, which the route
supplies to every template, so moving it to the consuming pane is safe.
Why the existing tests missed it
Every Admin Settings test inspects the template as text: field names,
card ids, tag balance, navigation parity, modal placement. All of them
passed, because none of them execute the template.
test_admin_settings_renders.py now renders the whole page through
Jinja with the same undefined handling Flask uses, so an
UndefinedError surfaces in the suite instead of in a browser. It also
asserts every navigation tab renders a pane and that exactly one pane
is active and it is the landing tab. The context is derived from the
route's own render_template call and the app context processors, so it
cannot drift.
test_admin_settings_pane_variable_scope.py adds static scope analysis
via jinja2.meta, which catches the silent variant a render cannot,
and names the offender precisely:
'analyze_capability' is used in 'actions' but only declared in ['agents']
Both are verified against a planted copy of the real bug.
The rule: a template is not verified until it has been rendered.
Verified
page renders 1,383,741 chars, no error
tabs -> panes 44 / 44
active panes exactly 1, and it is the landing tab
field names 452 on Development, 452 here, none lost, none added
regression set 32 failures, identical to baseline
jinja compile 46/46 admin templates
xss sinks pass
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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 freeto 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.
Fixes the 500 on Admin Settings, plus a silent variant of the same bug found while investigating.
Base:
feature/admin-settings-ia.The failure
Root cause
Jinja
{% set %}scope does not cross an{% include %}boundary.While Admin Settings was one 14,000-line template this never mattered — a variable derived near the top was visible everywhere below it. Splitting it into 44 per-tab partials made it matter: a variable derived in one partial is simply not visible in another.
Three variables were left behind when their consuming card moved to a different tab:
analyze_capabilityagentsactionscomparison_capabilityagentsactionsenable_dai_debugredis-cachingcosmosEach is a pure derivation from
settings, which the route supplies to every template, so moving the declaration into the consuming pane is safe and self-contained.The one you didn't see
The two failure modes are very different, and the quiet one is worse:
Undefinedis falsy.enable_dai_debugis used as{% if enable_dai_debug %}in six places on the Cosmos tab. Those debug controls and shadow-validation diagnostics would never have rendered, whatever the setting was, and nothing would have reported a problem. It was found only by auditing for the pattern that caused the crash — not by the error.Why every existing test passed
All the Admin Settings tests inspect the template as text: field names, card ids, tag balance, navigation parity, modal placement. Every one was green.
None of them execute the template. A template can satisfy every static check and still raise the moment Flask renders it. That is the real gap this PR closes.
test_admin_settings_renders.pyRenders the whole page through Jinja using the same undefined handling Flask uses, so an
UndefinedErrorsurfaces in the suite instead of in a browser. This is the test that would have caught the bug immediately.It also asserts every navigation tab renders a pane, and that exactly one pane is active and it is the landing tab.
The render context is derived from the route's own
render_templatecall and the app context processors, so it cannot drift as those change.test_admin_settings_pane_variable_scope.pyStatic scope analysis via
jinja2.meta.find_undeclared_variables, which catches the silent variant a render cannot. It names the offender precisely:Variables declared inside a
{% for %}loop are correctly treated as local.Both tests are verified against a planted copy of the real bug, so they are known to fail when they should.
Verification
DevelopmentNo setting changed and nothing needs re-entering.
Version 0.260.019. Full write-up in ADMIN_SETTINGS_PANE_VARIABLE_SCOPE_FIX.md.
The rule worth keeping: a template is not verified until it has been rendered.