feat: warn when a variables key reads a later key in the same block - #2084
feat: warn when a variables key reads a later key in the same block#2084Patch76 wants to merge 3 commits into
Conversation
HA renders a `variables:` block one key at a time, feeding each rendered
value into the context for the next (`ScriptVariables.async_simple_render`
for an action-level step, `async_render` for the top-level blocks). A key
whose template reads a sibling declared further down therefore renders
against a name that is not defined yet. HA's default undefined is
non-strict, so rather than raising it yields an empty string and a falsy
boolean: the automation loads, runs, and carries on through the wrong
branch, leaving only a template-variable warning in the log and on the
trace.
`variables` is a documented legitimate template position and is not walked
by any recursion path, so the ordering scan needs a pass of its own. It
runs on action-level `variables:` steps, on the top-level `variables:` and
`trigger_variables:` blocks of an automation, and on the top-level
`variables:` of a script.
Reaching action-level steps wherever they can sit took one fix to the
action-tree walk: a `sequence:` grouping action was never recursed into,
and since HA normalises a shorthand `parallel:` branch list into
`{"sequence": ...}`, the canonical form of both was invisible. That arm now
walks, which incidentally extends the pre-existing detectors into the same
two shapes.
Only later siblings count. A name coming from an earlier sibling, an
earlier action's `response_variable`, or the trigger context is legal and
is never flagged. Matching is on Jinja identifier tokens, so a sibling's
name inside a string literal, behind a dot, in a filter position, as part
of a longer identifier, or bound by an inline `{% set %}` is not read as a
reference. The false positives that remain, and the one gap where HA merges
both top-level blocks into a single rendered mapping, are enumerated on
`_check_variables_order`.
This does not stop the key reordering the issue reports. On the write path
a `variables:` block arrives in the order it was sent.
`_normalize_automation_config` re-inserts a popped key in two places: the
canonical plural root keys, which reorders the root mapping only, and
`sequences` -> `sequence`, which does apply at any depth but only fires on
a key literally named `sequences`. HA's own
`EditAutomationConfigView._write_value` lifts root keys to the front, again
root-only. The sort the issue describes therefore happens above the MCP
server. The check fires on whatever order the block arrives in, which is
why it is worth having regardless.
Closes homeassistant-ai#2072
|
@codex review — apply the review criteria in .gemini/styleguide.md in addition to AGENTS.md guidance |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53d7225274
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four defects from the first review round, each reproduced against the
checker before and after the change.
Local `{% set %}` bindings are now applied by position instead of being
collected up front. A read that happens before the binding is a genuine
forward reference — HA evaluates it before the name is bound — and the
up-front collection silently swallowed it. Only the right-hand side of a
binding is scanned, so the target name itself is not counted as a read.
`{{ later }}{% set later = 1 %}` and `{% set later = later %}` now warn;
`{% set total = 1 %}{{ total }}` still does not.
String literals are matched with escape awareness, and with `re.DOTALL` so
an escaped newline is consumed with the literal the way Jinja's own
`string_re` does. Without either, the literal ended early and its tail was
scanned as code, reporting a name that is only text as a read.
Identifiers are matched as `[^\W\d]\w*` rather than `[A-Za-z_]\w*`. Jinja
identifiers are `\w`-based, so `über` and `变量` are valid variable names;
the ASCII-anchored pattern returned no token for them and the check went
silent on exactly the localized names this bug tends to involve.
`then`, `else`, `default` and a nested `sequence` now accept a lone action
mapping as well as a list, as does `parallel`. The first four are
`SCRIPT_SCHEMA` positions and that schema is
`vol.All(ensure_list, [script_action])`; `parallel` has its own
`vol.All(ensure_list, [...])` that coerces the same way. A single mapping
is therefore valid wherever a list is. `repeat.sequence` and
`choose[].sequence` already handled it by routing through
`_check_action_tree`.
The `{% set %}` pattern also gets possessive quantifiers. Its `\s`, the
run between, and the trailing `\s` all match whitespace, which made a long
space run inside `{% set %}` cost cubic time — 800 spaces took a second,
and it grew eightfold per doubling. It is now flat.
One reported case is not fixed and is documented instead: a `}}` or `%}`
inside a string literal ends the span early, hiding the rest of the
expression. It cuts both ways — it can hide a real forward read and it can
warn about a name that is only text. Closing it needs a real Jinja
tokenizer, which is more machinery than an advisory check justifies.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The possessive rewrite used `*+` where the original pattern used `+?`, so
an invalid `{% set = 1 %}` captured an empty target instead of not matching
at all. Nothing downstream is affected — an empty string never equals a
variables key — but `++` removes the need for that argument.
Verified identical to the original lazy pattern across 18 `{% set %}`
shapes, including namespace assignment, the block form, unicode targets and
an `=` inside a string literal. Runtime stays flat: 25600 spaces in 0.3 ms.
|
@codex review — apply the review criteria in .gemini/styleguide.md in addition to AGENTS.md guidance |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70274f027a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if binding: | ||
| local.update(n.strip() for n in binding.group(1).split(",")) |
There was a problem hiding this comment.
The positional fix still treats every {% set %} target as local for the rest of the template, even when the assignment is scoped inside a Jinja loop or macro. For example, {% for x in [1] %}{% set later = 1 %}{% endfor %}{{ later }} followed by a sibling key named later produces no warning, although the loop-local assignment does not survive after endfor, so the final expression still performs the forward read. Track Jinja block scope when accumulating bindings, or suppress only assignments known to remain in the current scope.
Useful? React with 👍 / 👎.
| for block_key in ("variables", "trigger_variables"): | ||
| _check_variables_order(config.get(block_key), warnings, skill_prefix, block_key) |
There was a problem hiding this comment.
When an automation has both trigger_variables and variables, HA merges them before sequential rendering, but this loop scans the two original mappings independently. For example, if trigger_variables contains first: "{{ later }}" followed by later: 1, while variables overrides first with a literal, the effective merged mapping never renders the forward reference but this code still emits a warning from the discarded value; conversely, a reference crossing from trigger_variables into a later variables key is missed. Reproduce HA's merge and overwrite ordering before running the ordering check.
Useful? React with 👍 / 👎.
kingpanther13
left a comment
kingpanther13
left a comment
There was a problem hiding this comment.
Requesting changes. Everything below is reproduced against 70274f02 by loading the checker standalone. CI is green and I did not re-run it.
1. The warning states a consequence that's wrong for most shapes. best_practice_checker.py:899-905, same claim at :850-853.
"resolves to an empty, falsy value instead of erroring" holds only for bare {{ later }} and {% if later %}. HA's undefined is LoggingUndefined(jinja2.Undefined) (homeassistant/helpers/template/__init__.py:696) — not ChainableUndefined — and its _fail_with_undefined_error re-raises via super(). Only __str__ and __bool__ are non-raising, so {{ later.state }}, {{ later['k'] }}, {{ later + 1 }}, {{ later > 5 }} and {{ later | int }} raise UndefinedError, log at ERROR, and abort the step. The subscript form is the one this PR's own negative test uses at test_best_practice_checker.py:2561. Please state both outcomes — an agent reading "instead of erroring" won't connect a real UndefinedError in the log back to this warning.
2. A variables: key named sequences defeats the check and loses user data. tools_config_automations.py:140 applies field_mappings["sequences"] = "sequence" at every depth — only the trigger/action/condition mappings are is_root-gated — and the checker runs on the already-normalized dict (:808 → :821). Verified:
variables: {sequences: "{{ other }}", other: "5"}→ renamed and moved to the end; checker returns 0 warnings.variables: {sequences: "…", sequence: "keep", other: "5"}→ hitsdel normalized[src]; the user's variable is silently dropped before the write.
Independently of this check, a user variable named sequences is renamed in what gets written to HA, breaking every {{ sequences }} reference in the automation. The alias mapping shouldn't apply inside a variables:/trigger_variables: mapping — that's a free-form user namespace, not HA's action schema.
3. :876 misattributes the merge. trigger_variables + variables are merged in _create_automation_entities (core components/automation/__init__.py:1077-1087), not _async_process_config, which starts at 1107 and only reaches it by calling that function.
4. False positives on valid configs. The docstring enumerates known FPs, so these read as omissions. All reproduced end-to-end:
{# {{ b }} #}—_RE_TEMPLATE_SPAN(:165) has no{#…#}arm, so a commented-out template is scanned as live code.{% with b = 3 %}—_RE_JINJA_SET(:179) matches onlyset.{%+ set b = 3 %}—\{%-?allows only-; Jinja also accepts+. The{%-form is clean, so it's the marker.{% set ns.x = 1 %}and{% set (b, c) = (1, 2) %}— targets the character class can't consume make the span fall through, sons/b/ccount as reads.{{ wetter . b }}—:184-186claims the lookbehind covers "a preceding dot (wetter.meldung)"; it covers the unspaced form only. (Checker behavior verified; that Jinja parses the spaced form as getattr is from readingparse_postfix, not executed.){% for %}bindings, macro parameters, block-form{% set x %}…{% endset %}, call keyword args, and Jinja test names afterisall still fire.
(?:set|with) and [-+]? at :179 plus a {#…#} arm at :165 closes most of it.
5. Untested branches. Each could break with the suite green: _iter_strings' dict-key recursion (:813 — no test puts a template in a key), multi-target {% set a, b = %} (every set test binds one name), and the {%- set form. Nothing ties _normalize_automation_config to the checker either, which is the seam the "fires on arrival order" premise rests on — worth pinning given item 2.
6. :907 points at automation-patterns.md#variables. The anchor resolves so the anchor test passes, but that section is a DRY blurb plus a trigger_variables note — nothing on per-key sequential rendering. The embedded skill content contributes nothing to this warning; that section needs a sentence on ordering.
Two I'd rather you decide than assume:
- Module size. 994 → 1167 lines, past the ~1000-line mark in AGENTS.md § Module Size. The ordering pass at
:796-908with its five module-level regexes is a clean seam if you want it split here. - The
{{ "}}" ~ later }}truncation. Closing it needs literal-before-span tokenization.jinja2isn't inuv.lock, so it's a hand-rolled lexer or a new runtime dependency for standaloneuvx ha-mcp— your call which, or whether the documented limitation stands.
Confirmed working, for the record: the sequence/parallel arm adds warnings only where a detector already in the file was blind to the shape (bare sequence:, canonical {sequence:} parallel branch, singleton dicts) — ordinary configs stay at zero, no double-emission. Positional {% set %} handling, wiring across all four render positions, and totality of _iter_strings/_referenced_names all check out.
|
@Patch76 to invoke a codex review from the thing Julien set up you just type /**review (remove asterisks, I don't want to trigger another). Otherwise it'll try going to your account and won't work. On that note please resolve the pending codex reviews on your next round lol |
3 participants
Closes #2072
What does this PR do?
Adds a best-practice warning for a
variables:block whose key reads a sibling declared later in the same block.HA renders a variables block one key at a time, feeding each rendered value into the context for the next (
ScriptVariables.async_simple_renderfor an action-level step,async_renderfor the top-level blocks). A key that reads a name declared further down therefore renders against a name that is not defined yet. HA's default undefined is non-strict, so rather than raising it yields an empty string and a falsy boolean: the automation loads, runs, and carries on through the wrong branch, leaving only a template-variable warning in the log and on the trace.variablesis a documented legitimate template position and is not walked by any recursion path, so this is a separate ordering-only pass. It runs on action-levelvariables:steps, on an automation's top-levelvariables:andtrigger_variables:, and on a script's top-levelvariables:.Only later siblings count — a name from an earlier sibling, an earlier action's
response_variable, or the trigger context is legal and is never flagged. Matching is on Jinja identifier tokens, so a sibling's name inside a string literal, behind a dot, in a filter position, as part of a longer identifier, or bound by an inline{% set %}is not read as a reference.Two things I want review on:
An extra arm in the action-tree walk. A
sequence:grouping action was never recursed into, and since HA normalises a shorthandparallel:branch list into{"sequence": ...}(_SCRIPT_PARALLEL_SCHEMA/_parallel_sequence_actioninhomeassistant/helpers/config_validation.py), the canonical form of both was invisible to the walk. Without that arm the new check silently misses those shapes. Adding it also extends the pre-existing detectors into the same two shapes — a widening of correct detection rather than a semantic change, and the existing tests are unchanged by it. Say the word if you would rather have it as its own PR.Why a regex scan and not a real parse.
jinja2.meta.find_undeclared_variableswould drop every token-level false positive below — it reports only names resolved from context, so keywords, tests, globals,{% for %}bindings and both{% set %}forms fall away. It would not drop the outer-scope one, which needs a scope walk rather than a parser. jinja2 is not a dependency and is not inuv.lock, and pulling a template engine into the runtime for one advisory warning seemed the wrong trade against a scan that matches every other detector in this file. Happy to change course if you see it differently.The main false positives are enumerated on
_check_variables_orderrather than engineered away: a later sibling whose name also exists in an outer scope (the warning text names that case), a sibling colliding with a Jinja keyword, test, global or a{% for %}binding, a sibling shadowed by the block form{% set x %}...{% endset %}, and a sibling name used as a call keyword argument or sitting inside a{% raw %}body. All of them need a later sibling with that exact name to bite. One gap is documented in the same place: when an automation declares both top-level blocks, HA merges them into a single sequentially-rendered mapping, so atrigger_variableskey reading avariableskey is a forward read this per-block scan does not see.This does not stop the reordering the issue reports. On the write path a
variables:block arrives in the order it was sent._normalize_automation_configre-inserts a popped key in two places — the canonical plural root keys, which reorders the root mapping only, andsequences→sequence, which does apply at any depth but only fires on a key literally namedsequences. HA's ownEditAutomationConfigView._write_valuelifts root keys to the front, again root-only. So the sort the issue describes happens above the MCP server, and the check fires on whatever order the block arrives in, which is why it holds regardless of where that sort came from.Type of change
Testing
uv run pytest)uv run ruff check)So the two unticked boxes are not a mystery — what I actually ran:
test_relay_timeout_upstream_contract.py) is excluded because it cannot introspect aiohttp on this musl host; it errors identically on an unmodifiedmaster.ruff check,ruff formatandmypyover the changed files.Each discriminating condition in the new pass was mutation-tested — broken in turn, with the test that should fail confirmed to fail. (The
len(variables) < 2early return is an equivalent mutant, not a semantic condition: for a one-key block the loop is empty anyway.) Two of those runs found real problems: one exclusion was attributed to the wrong part of the regex, and an optimisation had quietly made one negative test non-discriminating.Beyond the unit tests, the reporter's exact config shape was run through
_normalize_automation_configand the checker together, which confirms the seam the whole check depends on: normalisation leaves the nested block order untouched, so the warning fires on the order that was actually submitted.Checklist