Uh oh!
There was an error while loading. Please reload this page.
Add a value-level render assertion to the pane scope tests - #1323
Open
Paul Lizer (paullizer) wants to merge 2 commits into
Open
Add a value-level render assertion to the pane scope tests#1323Paul Lizer (paullizer) wants to merge 2 commits into
Paul Lizer (paullizer) wants to merge 2 commits 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>Folds in the stronger check from a parallel fix of the same bug. Scope analysis proves a name resolves. It does not prove the value arrives: a pane could declare a variable locally, satisfying every scope check, and still derive it from the wrong place. Rendering the actions pane with real settings and asserting 25, 250, 10 and 100 reach their inputs closes that gap. Both new test files now call assert_app_version_at_least, matching the repo convention for version-aware functional tests. 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
1 participant
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.
Test-only follow-up to #1322, which carries the actual fix. Opened from a fork because write access to the upstream branch was revoked mid-session.
Merge #1322 first — this stacks a stronger assertion on top of it.
Why
Scope analysis proves a name resolves. It does not prove the value arrives.
A pane could declare a variable locally, satisfying every check in #1322, and still derive it from the wrong place — say
settings.document_action_capabilities.comparisonwhereanalyzewas meant. Every scope test would pass and the wrong numbers would render.This adds a render of the actions pane with real settings, asserting the values reach their inputs:
then asserts all four values appear in the output.
Credit where due: this assertion came from your own parallel fix of the same bug on
E:— it was the one thing my version was missing.Also
Both new test files now call
assert_app_version_at_least("0.260.019"), matching the repo convention for version-aware functional tests.Verification
test_admin_settings_pane_variable_scope.pytest_admin_settings_renders.pyNo application code changes — tests only.