Skip to content

Validate how a global is called, not just that it exists (#104) - #451

Merged
jeremymanning merged 1 commit into
mainfrom
fix/global-call-validation
Aug 3, 2026
Merged

Validate how a global is called, not just that it exists (#104)#451
jeremymanning merged 1 commit into
mainfrom
fix/global-call-validation

Conversation

@jeremymanning

Copy link
Copy Markdown
Member

Addresses the finding on #450: the validators recognise global names but not how they are used.

Confirmed, plus three more

written before run time
{{ nowx() }} rejected
{{ now.foo }} accepted fails
{{ file_exists.bad }} accepted fails
{{ now(1, 2, 3) }} accepted fails
{{ file_exists() }} accepted fails
{{ active_loops.x.y }} accepted fails
{{ now }} accepted succeeds

The last row is the one worth the PR. Nothing fails — the repr of a live function object is written into the artifact where a timestamp was meant to go:

<function TemplateManager._setup_custom_filters.<locals>.now at 0x1084...>

It is not the #447 escape returning. I checked before assuming:

{{ now.__globals__ }}    -> SecurityError
{{ now.__class__ }}      -> SecurityError
{{ now.__call__() }}     -> SecurityError

What leaks is the repr, not the object graph. Tests pin that, at render time rather than validation time, so it holds for templates that never met the validator.

Why the AST

The text cannot tell these apart — now is spelled identically in {{ now() }}, {{ now.foo }} and {{ now }}. The check reads the parsed Jinja AST and asks whether each Name node is the callee of a Call. Identity, not spelling: {{ now() }}{{ now.foo }} has two nodes with the same name and only one is valid. Mutating that to a name lookup is mutation 3 below.

Ownership inverted, as recommended

core/template_globals.py declares the language — name, argument contract, summary — following the core/actions.py convention. #450 derived the names from whatever TemplateManager registered; that could not drift but could not express an arity either, and it made the public language a shadow of a private implementation detail.

Two tests keep what derivation gave for free: declared names must equal what the runtime registers, and each declared arity must match the callable's real signature (inspect.signature), so the contract cannot claim something the function does not do.

Conservative about shadowing

{% for now in items %}{{ now }}{% endfor %} binds the name rather than using ours. The binding is a store, but the use in the body is an ordinary load and is indistinguishable without tracking scope — so a template that binds the name anywhere is left alone. A false rejection here is precisely the failure the last three changes to this validator existed to remove. Covered for for, set and macro.

Also

  • Stable error codes (global_not_called, global_wrong_arity) rather than message matching.
  • docs/template_globals.md, generated from the same specs the validator checks against, with the --check drift test that docs/actions.md uses.

Measured

before after
blocking suite 616 passed 670 passed, 0 failed
catalogue validating 20 / 117 20 / 117 — new strictness rejects no example that validated before

Mutations — five, all killed

mutation tests that failed
unwire the check from the validator 4
disable the arity check 8
match the callee by name, not identity 1
declare file_exists as taking 0 args 4, incl. the docs check
drop the shadowed-name exclusion 3

The four wiring tests exist because without them, deleting the validator's call site leaves every other test in the file passing — they would be testing the function rather than the validation.

Next

Typed RuntimeContext. Groundwork from #450: seven sites build an execution dict in four timestamp formats, and execution.timestamp is rebuilt per task, so two steps in one run disagree (20:01:55.182681 vs 20:01:55.184368). now's doc entry already points at execution.timestamp for the deterministic case.

🤖 Generated with Claude Code

#450 taught both validators the eight global *names*, which stopped
`{{ now() }}` -- a pipeline that runs correctly -- from being reported as an
undefined variable. It stopped at the name, and so accepted every other way
of writing one:

    {{ nowx() }}            rejected   (not a global)
    {{ now.foo }}           accepted   -> fails at run time
    {{ file_exists.bad }}   accepted   -> fails at run time
    {{ now(1, 2, 3) }}      accepted   -> fails at run time
    {{ file_exists() }}     accepted   -> fails at run time
    {{ now }}               accepted   -> *runs*

The last one is the one worth the PR. Nothing fails: the repr of a live
function object is written into the artifact where a timestamp was meant to
go --

    <function TemplateManager._setup_custom_filters.<locals>.now at 0x1084...>

It is not the #447 escape returning: `now.__globals__` and `now.__class__`
are both refused by the sandboxed environment, and tests now pin that. What
leaks is the repr, not the object graph.

The text cannot tell these apart -- `now` is spelled identically in
`{{ now() }}`, `{{ now.foo }}` and `{{ now }}` -- so the check reads the
parsed Jinja AST and asks whether each Name node *is the callee of a Call*.
Identity, not spelling: `{{ now() }}{{ now.foo }}` has two nodes with the
same name, one valid.

`core/template_globals.py` declares the language: name, argument contract,
summary, following `core/actions.py`. That reverses #450, which derived the
names from whatever `TemplateManager` registered. Derivation could not drift
but could not express an arity either, and it made the public language a
shadow of a private implementation detail. Two tests keep what derivation
gave for free: the declared names must equal what the runtime registers, and
each declared arity must match the callable's real signature.

`{% for now in items %}{{ now }}{% endfor %}` binds the name rather than
using ours. The binding is a `store` but the use inside the body is an
ordinary `load`, indistinguishable without tracking scope, so a template that
binds the name anywhere is left alone. Deliberately conservative: a false
rejection here is the exact failure the last three changes to this validator
existed to remove.

Errors carry stable codes -- `global_not_called`, `global_wrong_arity` --
rather than matching on message text.

docs/template_globals.md is generated from the same specs the validator
checks against, following scripts/generate_action_docs.py, with the same
--check test so it cannot drift.

Measured: blocking suite 616 -> 670 passed, 0 failed. Catalogue unchanged at
20/117 -- the new strictness rejects no example that validated before.

Five mutations, all killed:
  - unwire the check from the validator      -> 4 tests
  - disable the arity check                  -> 8 tests
  - match the callee by name, not identity   -> 1 test
  - declare file_exists as taking 0 args     -> 4 tests, incl. the docs check
  - drop the shadowed-name exclusion         -> 3 tests

The wiring tests exist because without them, deleting the validator's call
site leaves every other test in the file passing -- the checks would be
testing the function rather than the validation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jeremymanning
jeremymanning merged commit 3e811d2 into main Aug 3, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant