From a6a1c8809a33025ac6716b2ad78ce9a453a7444f Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:06:46 -0700 Subject: [PATCH 1/4] fix: Enforce the 512-character cap on a let binding identifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Template Schemas §3.6.1 caps a `` at 512 characters. Nothing checked it, so a 513-character `let` binding name was accepted. The conformance fixture pinning this is `EXPR/job_templates/proposed/3.6.1--let-identifier-513.invalid.yaml`, added by openjd-specifications#164, which failed here and in openjd-rs alike. The cap is a flat 512 and deliberately does not reuse the §7.1 `` cap that `NameIdentifierLengthMixin` and the inline `512 if "FEATURE_BUNDLE_1" in context.extensions else 64` checks apply. §3.6.1 states one maximum for a `` and gates it on no extension, and the fixture pair proves the flat reading: the 512-character accept twin `EXPR/job_templates/3.6--let-boundary-edges.yaml` declares EXPR alone, so borrowing the §7.1 cap would limit it to 64 and reject a template the spec permits. The two caps share the number 512 and nothing else, so this adds `LET_MAX_IDENTIFIER_LEN` beside the existing `LET_MAX_BINDINGS` with its own citation. EXPR gates whether `let` exists at all and moves neither cap. All four `_validate_let` field validators route through `validate_let_field` and then `parse_let_bindings`, so the check goes in `parse_let_bindings` after the `_LET_NAME_RE` match and every scope is covered by one insertion. The message omits the offending name: at 513 characters it would dwarf the diagnostic. Six tests, mutation-checked: removing the check fails all three reject tests while the three accept tests keep passing, so the accept cases act as negative controls against a cap set too low. The default `_job` helper declares EXPR alone, which is what would catch a regression to the FEATURE_BUNDLE_1-gated cap; the `_with_fb1` variants pin that declaring FEATURE_BUNDLE_1 does not move it. The matching openjd-rs change is OpenJobDescription/openjd-rs#358. Design note: SuperDaveDocs docs/conformance-0901/4.6-let-513/fix.md Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/model/v2023_09/_model.py | 19 ++++++++ .../model_v0/v2023_09/test_let_bindings.py | 47 +++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index 56caa30c..dfe58404 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -921,6 +921,19 @@ class ScriptInterpreter(str, Enum): LET_MAX_BINDINGS = 50 _LET_NAME_RE = re.compile(r"^[a-z_][A-Za-z0-9_]*$") +# §3.6.1: the maximum length of a `let` binding's ``. +# +# Flat 512, deliberately not the §7.1 `` cap that +# `NameIdentifierLengthMixin` and the inline +# `512 if "FEATURE_BUNDLE_1" in context.extensions else 64` checks apply. §3.6.1 +# states one maximum for a `` and does not gate it on an +# extension, and the conformance pair proves the flat reading: +# `EXPR/job_templates/3.6--let-boundary-edges.yaml` declares `EXPR` alone and +# requires a 512-character name to be accepted, so borrowing the §7.1 cap would +# limit it to 64 and reject a template the spec permits. EXPR gates whether +# `let` exists at all and moves neither cap. +LET_MAX_IDENTIFIER_LEN = 512 + def parse_let_bindings(value: Any) -> list[tuple[str, str]]: """Parse a ``let`` field value (list of ``"name = expression"`` strings) @@ -941,6 +954,12 @@ 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}") + # The name is not interpolated into the message: at 513 characters it + # would dwarf the diagnostic. + if len(name) > LET_MAX_IDENTIFIER_LEN: + raise ValueError( + f"A 'let' binding name must be at most {LET_MAX_IDENTIFIER_LEN} characters long" + ) if not expr: raise ValueError(f"A 'let' binding must define an expression: {binding!r}") result.append((name, expr)) diff --git a/test/openjd/model_v0/v2023_09/test_let_bindings.py b/test/openjd/model_v0/v2023_09/test_let_bindings.py index ac0ddc6a..7bc07ca9 100644 --- a/test/openjd/model_v0/v2023_09/test_let_bindings.py +++ b/test/openjd/model_v0/v2023_09/test_let_bindings.py @@ -41,6 +41,25 @@ 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, because the cap does not depend on FEATURE_BUNDLE_1. + def test_name_512_chars(self): + name = "a" * 512 + _decode(_job([{"name": "S", "let": [f"{name} = 1"], "script": _onrun("hi")}])) + + def test_name_512_chars_with_fb1(self): + name = "a" * 512 + _decode( + _job( + [{"name": "S", "let": [f"{name} = 1"], "script": _onrun("hi")}], + 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("hi")}}])) + def test_chained_and_functions(self): _decode( _job( @@ -98,6 +117,34 @@ 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: a `` is at most 512 characters. The cap is flat, + # not the FEATURE_BUNDLE_1-gated §7.1 `` cap, so the accept case + # must hold with EXPR alone. `_job` declares EXPR only by default, which is + # what the conformance fixture + # `EXPR/job_templates/3.6--let-boundary-edges.yaml` asserts; the + # `_with_fb1` variants pin that declaring FEATURE_BUNDLE_1 does not move it. + def test_name_513_chars(self): + name = "a" * 513 + with pytest.raises(DecodeValidationError, match="at most 512 characters"): + _decode(_job([{"name": "S", "let": [f"{name} = 1"], "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( From ab33552f5a80ce1ee36200dc7ef5bc68bbd30ed0 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:05:15 -0700 Subject: [PATCH 2/4] chore: Shorten the let identifier cap comments The rationale lives in the commit message, the PR description and the design note. The code only needs the citation and the one fact a reader editing this line must not miss: the cap is flat, so it is not the FEATURE_BUNDLE_1-gated section 7.1 cap. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/model/v2023_09/_model.py | 17 ++++------------- .../model_v0/v2023_09/test_let_bindings.py | 12 ++++-------- 2 files changed, 8 insertions(+), 21 deletions(-) diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index dfe58404..32cfc0e7 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -921,17 +921,9 @@ class ScriptInterpreter(str, Enum): LET_MAX_BINDINGS = 50 _LET_NAME_RE = re.compile(r"^[a-z_][A-Za-z0-9_]*$") -# §3.6.1: the maximum length of a `let` binding's ``. -# -# Flat 512, deliberately not the §7.1 `` cap that -# `NameIdentifierLengthMixin` and the inline -# `512 if "FEATURE_BUNDLE_1" in context.extensions else 64` checks apply. §3.6.1 -# states one maximum for a `` and does not gate it on an -# extension, and the conformance pair proves the flat reading: -# `EXPR/job_templates/3.6--let-boundary-edges.yaml` declares `EXPR` alone and -# requires a 512-character name to be accepted, so borrowing the §7.1 cap would -# limit it to 64 and reject a template the spec permits. EXPR gates whether -# `let` exists at all and moves neither cap. +# §3.6.1: maximum length of a `let` binding's ``. 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 @@ -954,8 +946,7 @@ 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}") - # The name is not interpolated into the message: at 513 characters it - # would dwarf the diagnostic. + # Name omitted from the message; at 513 characters it would dwarf it. if len(name) > LET_MAX_IDENTIFIER_LEN: raise ValueError( f"A 'let' binding name must be at most {LET_MAX_IDENTIFIER_LEN} characters long" diff --git a/test/openjd/model_v0/v2023_09/test_let_bindings.py b/test/openjd/model_v0/v2023_09/test_let_bindings.py index 7bc07ca9..ed527d26 100644 --- a/test/openjd/model_v0/v2023_09/test_let_bindings.py +++ b/test/openjd/model_v0/v2023_09/test_let_bindings.py @@ -41,8 +41,8 @@ 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, because the cap does not depend on FEATURE_BUNDLE_1. + # §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 _decode(_job([{"name": "S", "let": [f"{name} = 1"], "script": _onrun("hi")}])) @@ -117,12 +117,8 @@ 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: a `` is at most 512 characters. The cap is flat, - # not the FEATURE_BUNDLE_1-gated §7.1 `` cap, so the accept case - # must hold with EXPR alone. `_job` declares EXPR only by default, which is - # what the conformance fixture - # `EXPR/job_templates/3.6--let-boundary-edges.yaml` asserts; the - # `_with_fb1` variants pin that declaring FEATURE_BUNDLE_1 does not move it. + # §3.6.1 caps a `` 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="at most 512 characters"): From c476cf86ca850e5c09026648ebf41ef9eb836be6 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:21:50 -0700 Subject: [PATCH 3/4] fix: Name the over-long binding in the let identifier error Review finding on #348. The validator is a `field_validator` on the whole `let` list, so pydantic anchors the error at `steps[0] -> let` with no list index. Measured on a four-binding template where the third name is 513 characters: the error was steps[0] -> let: A 'let' binding name must be at most 512 characters long which identifies neither the binding nor its position. With 50 bindings allowed that is not diagnosable. Truncate rather than omit, as the reviewer suggested. The message now carries a 32-character prefix and the true length: A 'let' binding name must be at most 512 characters long: 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'... (513 characters) Total error text goes from 110 to 166 characters, so it stays bounded while becoming locatable. The original concern about a 513-character name dwarfing the diagnostic still holds against interpolating the name in full, which is why this truncates. Two assertions pin it, both mutation-checked: reverting to the bare message fails `test_name_513_chars` and the new `test_name_513_chars_names_the_offending_binding`, which uses a multi-binding `let` to cover the case the finding was about. No matching change in openjd-rs: its path is `steps[0] -> let[0]`, so the index already identifies the binding there. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/model/v2023_09/_model.py | 7 +++++-- test/openjd/model_v0/v2023_09/test_let_bindings.py | 12 +++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index 32cfc0e7..afe189be 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -946,10 +946,13 @@ 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}") - # Name omitted from the message; at 513 characters it would dwarf it. + # 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: raise ValueError( - f"A 'let' binding name must be at most {LET_MAX_IDENTIFIER_LEN} characters long" + 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}") diff --git a/test/openjd/model_v0/v2023_09/test_let_bindings.py b/test/openjd/model_v0/v2023_09/test_let_bindings.py index ed527d26..d35cada5 100644 --- a/test/openjd/model_v0/v2023_09/test_let_bindings.py +++ b/test/openjd/model_v0/v2023_09/test_let_bindings.py @@ -121,9 +121,19 @@ def test_self_reference(self): # 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="at most 512 characters"): + 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"): From 344135cff8e70948f8ea8320f4f5d870b064492b Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:37:32 -0700 Subject: [PATCH 4/4] test: Reference the bound name in the let length accept cases Review finding on #348. The three accept-side tests bound a 512-character name and never referenced it, so they proved `parse_let_bindings` does not raise but not that the binding is usable, which is the property the conformance fixture is about. Each now interpolates the bound name in the script args, so the name flows through the variable-reference validation and into the expression parser. All 24 tests still pass, which answers the finding's concern directly: no cap further down that path, in `Identifier`, `FormatString`, or the Rust expression layer, rejects a 512-character binding. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- test/openjd/model_v0/v2023_09/test_let_bindings.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/openjd/model_v0/v2023_09/test_let_bindings.py b/test/openjd/model_v0/v2023_09/test_let_bindings.py index d35cada5..64171b48 100644 --- a/test/openjd/model_v0/v2023_09/test_let_bindings.py +++ b/test/openjd/model_v0/v2023_09/test_let_bindings.py @@ -45,20 +45,24 @@ def test_script_let(self): # EXPR alone, since the cap does not depend on FEATURE_BUNDLE_1. def test_name_512_chars(self): name = "a" * 512 - _decode(_job([{"name": "S", "let": [f"{name} = 1"], "script": _onrun("hi")}])) + # 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("hi")}], + [{"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("hi")}}])) + _decode( + _job([{"name": "S", "script": {"let": [f"{name} = 1"], **_onrun(f"{{{{{name}}}}}")}}]) + ) def test_chained_and_functions(self): _decode(