fix(model): Enforce the 512-character cap on a let binding identifier by leongdl · Pull Request #358 · OpenJobDescription/openjd-rs

leongdl · 2026-09-02T19:07:33Z

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

steps[0] -> let[0]:
	name exceeds 512 characters.

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-model-for-python alike. I found it while running every open conformance-test PR against both implementations; 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_]*

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 `EffectiveLimits::max_identifier_len`

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

EffectiveLimits::max_identifier_len implements the §7.1 <Identifier> cap: 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 max_identifier_len 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 a separate MAX_LET_IDENTIFIER_LEN 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 template types carry a let field — SimpleAction, StepTemplate, StepScript, EnvironmentScript — and all of them funnel through validate_let_bindings, which has seven call sites: step scope, step script scope, job environment script, step environment script, environment template script, the SimpleAction bindings, and a second pass over step scope that runs only when a step declares hostRequirements. The check therefore goes inside that function and no call site changes.

It sits after the character-set check rather than replacing it, so a name that breaks both rules reports both.

The message omits the offending name. Every other message in this validator interpolates it (name '{name}' contains invalid characters.), but at 513 characters that buries the diagnostic, and limits.rs already reports its length caps without the value. That is a deliberate break from local symmetry and the one wording choice I would like an opinion on.

The runtime path evaluate_let_bindings in job/create_job/instantiate.rs is left alone. It runs on templates validation has already accepted, and rejecting there would surface as a job-creation error with no field path instead of a validation error with one.

Verification

cargo fmt --all -- --check, cargo clippy --all-features --all-targets --workspace -- -D warnings, and cargo test --workspace all pass.

Six new tests in crates/openjd-model/tests/integration/test_let_bindings.rs: 513 rejected and 512 accepted, each at step scope and script scope, plus the accept and the reject with EXPR alone and no FEATURE_BUNDLE_1. That last pair is what pins the section above; without it a regression to max_identifier_len 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, 3 accept tests pass

Out of scope

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

Two pre-existing defects next to this code, left alone to keep the change reviewable. The SimpleAction call site passes the step path rather than a let field path, so a bad SimpleAction binding reports as steps[0][0] with no bash -> let segment. And a step declaring both let and hostRequirements runs its bindings through the validator twice without deduplication, so a 513-character name in such a step reports the new message twice. This change inherits both rather than causing them; say the word if you would rather they were fixed here.

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

Template Schemas §3.6.1 caps a `<UserIdentifier>` 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-model-for-python alike. The cap is a flat 512 and deliberately does not reuse `EffectiveLimits::max_identifier_len`. That field carries the §7.1 `<Identifier>` cap, which is 64 rising to 512 with FEATURE_BUNDLE_1. §3.6.1 states one maximum for a `<UserIdentifier>` 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 a separate named constant carrying its own citation. All four `let`-bearing template types (SimpleAction, StepTemplate, StepScript, EnvironmentScript) funnel through `validate_let_bindings`, which has seven call sites, so the check goes inside that function and the call sites are untouched. The check sits after the character-set check so a name that breaks both rules reports both. The message omits the offending name: at 513 characters it would dwarf the diagnostic, and `limits.rs` already reports its length caps without the value. The runtime path `evaluate_let_bindings` is left alone. It runs on templates validation has already accepted, and rejecting there would surface as a job-creation error with no field path. 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. Two of the six pin the accept and reject cases with EXPR alone, which is what would catch a regression to `max_identifier_len`. Design note: SuperDaveDocs docs/conformance-0901/4.6-let-513/fix.md Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

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 EffectiveLimits::max_identifier_len. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

Two review findings on OpenJobDescription#358. Count characters, not bytes. §3.6.1 caps characters, and the charset check above this one does not `continue`, so a multi-byte name drew a spurious length error alongside the real one. Measured before fixing: a name of 200 `の` characters is 600 bytes, and the old check reported both `contains invalid characters.` and `exceeds 512 characters.` for a name well inside the cap. Name the binding in the message, matching every sibling check in this validator (`name '{name}' contains invalid characters.`). The earlier reasoning for omitting it was that 513 characters would dwarf the diagnostic; the sibling messages already accept that tradeoff, so consistency wins. The Python change keeps its 32-character truncation instead, because its error path is `let` with no index and the name is the only way to identify the binding there. Also fixes the test helper that could not pin what it claimed. Effective extensions are the intersection of the template's `extensions` list with the caller allowlist, and the helpers here emit `["EXPR"]`, so allowlisting FEATURE_BUNDLE_1 never activated it: `decode_ok_expr_only` was indistinguishable from `decode_ok` and its comment was wrong. Replaced with `job_with_step_let_fb1`, which declares FEATURE_BUNDLE_1 in the body, so the FB1-on and FB1-off cases are genuinely different profiles. Seven tests. Mutation-checked: reverting to `name.len()` fails only the new multi-byte test, and dropping `{name}` fails only the three reject tests. A third mutant, making the cap FB1-gated, does not compile, because `validate_let_bindings` receives an `ExprProfile` and never sees the model extension set. That regression is structurally unavailable without first threading limits through all seven call sites. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

Template Schemas §3.6.1 caps a `<UserIdentifier>` 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 `<Identifier>` 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 `<UserIdentifier>` 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>

* fix: Enforce the 512-character cap on a let binding identifier Template Schemas §3.6.1 caps a `<UserIdentifier>` 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 `<Identifier>` 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 `<UserIdentifier>` 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> * 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> * 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> * 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> --------- Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

leongdl requested a review from a team as a code owner September 2, 2026 19:07

leongdl mentioned this pull request Sep 2, 2026

fix: Enforce the 512-character cap on a let binding identifier OpenJobDescription/openjd-model-for-python#348

Merged