Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leongdl merged 4 commits into from
Sep 2, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a6a1c88
fix: Enforce the 512-character cap on a let binding identifier
leongdl Sep 2, 2026
ab33552
chore: Shorten the let identifier cap comments
leongdl Sep 2, 2026
c476cf8
fix: Name the over-long binding in the let identifier error
leongdl Sep 2, 2026
344135c
test: Reference the bound name in the let length accept cases
leongdl Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/openjd/model/v2023_09/_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,11 @@ class ScriptInterpreter(str, Enum):
LET_MAX_BINDINGS = 50
_LET_NAME_RE = re.compile(r"^[a-z_][A-Za-z0-9_]*$")

# §3.6.1: maximum length of a `let` binding's `<UserIdentifier>`. Flat, so not
# the §7.1 cap NameIdentifierLengthMixin applies: that one is 64 without
# FEATURE_BUNDLE_1, and a 512-character name must be accepted with EXPR alone.
LET_MAX_IDENTIFIER_LEN = 512

@github-actions github-actions Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 leongdl Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack and there is a rust PR.

@leongdl leongdl Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 github-actions Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def parse_let_bindings(value: Any) -> list[tuple[str, str]]:
"""Parse a ``let`` field value (list of ``"name = expression"`` strings)
Expand All @@ -941,6 +946,14 @@ def parse_let_bindings(value: Any) -> list[tuple[str, str]]:
expr = expr.strip()
if not _LET_NAME_RE.match(name):
raise ValueError(f"A 'let' binding name must be a valid identifier: {name!r}")
# Truncated rather than omitted: the caller is a field_validator on the
# whole list, so the error path is `let` with no index to identify which
# binding is over.
if len(name) > LET_MAX_IDENTIFIER_LEN:

@github-actions github-actions Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 expr:
raise ValueError(f"A 'let' binding must define an expression: {binding!r}")
result.append((name, expr))
Expand Down
57 changes: 57 additions & 0 deletions test/openjd/model_v0/v2023_09/test_let_bindings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,29 @@ def test_script_let(self):
_job([{"name": "S", "script": {"let": ["a = 2"], **_onrun("{{a}}")}}]),
)

# §3.6.1 boundary: 512 characters is the maximum and must be accepted, with
# EXPR alone, since the cap does not depend on FEATURE_BUNDLE_1.
def test_name_512_chars(self):
name = "a" * 512
# Referenced, not just declared: a cap further down the path would
# otherwise be invisible here.
_decode(_job([{"name": "S", "let": [f"{name} = 1"], "script": _onrun(f"{{{{{name}}}}}")}]))

def test_name_512_chars_with_fb1(self):
name = "a" * 512
_decode(
_job(
[{"name": "S", "let": [f"{name} = 1"], "script": _onrun(f"{{{{{name}}}}}")}],
extensions=("EXPR", "FEATURE_BUNDLE_1"),
)
)

def test_name_512_chars_script(self):
name = "a" * 512
_decode(
_job([{"name": "S", "script": {"let": [f"{name} = 1"], **_onrun(f"{{{{{name}}}}}")}}])
)

def test_chained_and_functions(self):
_decode(
_job(
Expand Down Expand Up @@ -98,6 +121,40 @@ def test_self_reference(self):
with pytest.raises(DecodeValidationError, match="cannot reference itself"):
_decode(_job([{"name": "S", "let": ["x = x + 1"], "script": _onrun("hi")}]))

# §3.6.1 caps a `<UserIdentifier>` at 512 characters. `_job` declares EXPR
# alone, so these pin the cap independently of FEATURE_BUNDLE_1.
def test_name_513_chars(self):
name = "a" * 513
with pytest.raises(
DecodeValidationError,
match=r"at most 512 characters long: 'a{32}'\.\.\. \(513 characters\)",
):
_decode(_job([{"name": "S", "let": [f"{name} = 1"], "script": _onrun("hi")}]))

def test_name_513_chars_names_the_offending_binding(self):
# The validator is a field_validator on the whole list, so the error path
# is `let` with no index; the message has to identify the binding itself.
name = "b" * 513
with pytest.raises(DecodeValidationError, match=r"'b{32}'\.\.\. \(513 characters\)"):
_decode(_job([{"name": "S", "let": ["ok = 1", f"{name} = 2"], "script": _onrun("hi")}]))

def test_name_513_chars_with_fb1(self):
name = "a" * 513
with pytest.raises(DecodeValidationError, match="at most 512 characters"):
_decode(
_job(
[{"name": "S", "let": [f"{name} = 1"], "script": _onrun("hi")}],
extensions=("EXPR", "FEATURE_BUNDLE_1"),
)
)

def test_name_513_chars_script(self):
name = "a" * 513
with pytest.raises(DecodeValidationError, match="at most 512 characters"):
_decode(
_job([{"name": "S", "script": {"let": [f"{name} = 1"], **_onrun("hi")}}]),
)

def test_comprehension_shadows_let(self):
with pytest.raises(DecodeValidationError, match="shadows"):
_decode(
Expand Down
Loading