fix: Enforce the 512-character cap on a let binding identifier

[leongdl]

What

Template Schemas §3.6.1 caps a let binding's <UserIdentifier> at 512 characters. Nothing checked it, so a 513-character name was accepted.

steps:
- name: Step1
  let:
  - aaa…aaa = 42        # 513 characters: now rejected
  - aaa…aaa = 42        # 512 characters: still accepted

A 'let' binding name must be at most 512 characters long

Why

The conformance fixture pinning this is EXPR/job_templates/proposed/3.6.1--let-identifier-513.invalid.yaml, added by openjd-specifications#164. It fails on this implementation and on openjd-rs alike. I found it while running every open conformance-test PR against both implementations, each built from source; it is root cause F-6 of that sweep.

Spec text, pinned at the commit the sweep measured against — wiki/2023-09-Template-Schemas.md, L1424-L1454:

<UserIdentifier> ::= [a-z_][A-Za-z0-9_]*

3. Maximum length of <UserIdentifier>: 512 characters.

That is L1430 and L1441. The section states one maximum and does not gate it on any extension.

Why this does not reuse the §7.1 identifier cap

This is the decision worth reviewing, because reaching for the existing cap is the obvious move and it is wrong here.

This package already carries the §7.1 <Identifier> cap in NameIdentifierLengthMixin and in seven inline 512 if "FEATURE_BUNDLE_1" in context.extensions else 64 checks. That cap is 64 characters, rising to 512 with FEATURE_BUNDLE_1. §3.6.1's <UserIdentifier> is a different type with a flat maximum.

The conformance pair proves the flat reading rather than merely suggesting it. The 512-character accept twin, EXPR/job_templates/3.6--let-boundary-edges.yaml from openjd-specifications#160, declares extensions: [EXPR] with no FEATURE_BUNDLE_1. Under the §7.1 cap its limit would be 64, so a fix built on that cap would reject a template the spec permits and turn a passing fixture red.

The two caps share the number 512 and nothing else, so this adds LET_MAX_IDENTIFIER_LEN beside the existing LET_MAX_BINDINGS, carrying its own citation and a comment recording why it is not the other one. EXPR gates whether let exists at all and moves neither cap.

Where the check goes, and why one place is enough

Four models carry a let field, and all four _validate_let field validators route through validate_let_field and then parse_let_bindings. The check therefore goes in parse_let_bindings, immediately after the _LET_NAME_RE match, and every scope is covered by one insertion.

The message omits the offending name, because at 513 characters it would dwarf the diagnostic.

The runtime helper evaluate_let_bindings is left alone. It runs on templates validation has already accepted, so rejecting there would surface too late.

Verification

Full suite: 5531 passed, 24 skipped, 3 xfailed. ruff check and mypy clean. ruff format --check reports both files I touched as already formatted.

Two pre-existing failures are unrelated to this change: TestYAMLLoaderPerformance::test_extra_large_template_performance in both model_v0 and model_v1 asserts a hard 5x speedup and lands at 4.0x-4.9x on my machine. I confirmed they are pre-existing by stashing this change and re-running on unmodified mainline, where both still fail, at 4.0x and 4.2x.

Six new tests in test/openjd/model_v0/v2023_09/test_let_bindings.py: 513 rejected and 512 accepted, each at step scope and script scope, plus both cases with FEATURE_BUNDLE_1 declared. The file's _job helper declares EXPR alone by default, so the default variants are the ones that pin the section above; without them a regression to the FEATURE_BUNDLE_1-gated cap would pass the suite.

Mutation-checked. Removing the production check fails all three reject tests and leaves all three accept tests passing, so the accept cases act as negative controls against a cap set too low.

Mutant	Result
length check removed	3 reject tests fail, 20 pass

I cleared __pycache__ between runs and confirmed the mutated tree still imports, so the failures are the assertion firing rather than a broken build.

Out of scope

The §3.6.1 minimum length of 1 character. _LET_NAME_RE already rejects an empty name, and the conformance suite covers it.

Related

• openjd-specifications#164 — the fixture, currently parked in proposed/. It can move out once both implementations release the fix.
• The matching openjd-rs change: fix(model): Enforce the 512-character cap on a let binding identifier openjd-rs#358
• Design note: SuperDaveDocs docs/conformance-0901/4.6-let-513/fix.md

[github-actions]

Parity note: this cap lands only in the v0 pure-Python path, and there is no corresponding coverage on the Rust-backed side. test/openjd/model_v1/test_let_bindings.py has no length-boundary test, and test_known_gaps.py records no gap for it — so if openjd-model does not enforce a 512-character <UserIdentifier> cap, the two implementations now silently disagree on a template the spec section this PR cites is specifically about.

AGENTS.md calls out reference parity as a tracked artifact and test_known_gaps.py as the place divergences get recorded (xfail, driven to zero). Either a matching v1 test or a known_gaps entry would keep this from drifting unnoticed; a v0-only cap with nothing on the v1 side is the shape that parity tracking exists to catch.

[leongdl]

Ack and there is a rust PR.

[leongdl]

Parity is covered by OpenJobDescription/openjd-rs#358, which adds the same cap on the Rust side with its own seven tests, so the two implementations agree rather than diverging.

On the tracking ask specifically: not adding a known_gaps entry, because there is no gap left to record once #358 merges, and an xfail that never fails is worse than nothing. Not adding a v1 test either, since the v1 surface is evaluate_let_bindings, which runs on already-validated templates and does not perform this check in either implementation.

The honest residue is a release-ordering window: this repo's cap lands with this PR, the Rust one lands with #358, and they release independently, so between the two releases the pinned behaviour differs by version. The conformance fixture that covers this is parked in proposed/ in openjd-specifications#164 for exactly that reason and moves out once both have shipped. Happy to add a tracking entry instead if you would rather that window were recorded here.

[github-actions]

The length check is placed after the _LET_NAME_RE check, which defeats the truncation this block exists to provide. The comment above says the name is truncated to 32 chars so the error stays readable, but line 948 (A 'let' binding name must be a valid identifier: {name!r}) interpolates the full, untruncated name — and that is the branch a long name hits whenever it also contains a disallowed character or starts with a digit/uppercase.

So let: ["A" * 100000 + " = 1"] (uppercase first char → fails the regex) produces a ~100 KB ValueError that pydantic wraps into the DecodeValidationError message, while the same name lowercased produces the nice 32-char-truncated one. The over-long case that is most likely to be adversarial is exactly the one that skips the guard.

Moving the length check above the regex check fixes both: nothing over 512 characters ever reaches an interpolation site.

        if len(name) > LET_MAX_IDENTIFIER_LEN:
            raise ValueError(
                f"A 'let' binding name must be at most {LET_MAX_IDENTIFIER_LEN} "
                f"characters long: {name[:32]!r}... ({len(name)} characters)"
            )
        if not _LET_NAME_RE.match(name):
            raise ValueError(f"A 'let' binding name must be a valid identifier: {name!r}")

Worth noting {binding!r} at lines 941 and 957 is unbounded for the same reason (pre-existing), so the reordering only closes the name path — but the name path is the one this PR is adding a bound for.

[github-actions]

Scope question on the cap: parse_let_bindings is only reached from the four let field validators (lines 1077, 1127, 1174, 3558). But <UserIdentifier> in §3.6.1 also covers the names bound inside an expression — comprehension variables and inline let ... in ... bindings — e.g. {{ [aaaa… for aaaa… in Param.Items] }}. Those go through ExprNode/the Rust engine (self._parsed.local_bindings in _format_strings/_nodes.py), which this check never sees.

If the intent is "enforce the §3.6.1 identifier cap," the two forms should agree; if the intent is narrower — only the let field, because the engine already bounds its own binders — the constant name and comment are misleading, since LET_MAX_IDENTIFIER_LEN reads as the identifier limit generally. Either extending the check to local_bindings or narrowing the comment to say the engine owns the in-expression form would remove the ambiguity.

test_comprehension_shadows_let shows the comprehension path is already exercised in this file, so a boundary test there would be cheap if the cap is meant to apply.