fix: Make task parameter type names case-insensitive under EXPR - #350
Open
leongdl wants to merge 1 commit into
Open
fix: Make task parameter type names case-insensitive under EXPR#350leongdl wants to merge 1 commit into
leongdl wants to merge 1 commit into
Conversation
Template Schemas §2 says "job parameter and task parameter type names become case-insensitive" when the EXPR extension is enabled, and so are case-sensitive without it. `_normalize_parameter_type_case` already implemented that, gated on the extension, but it was registered only on `parameterDefinitions` in `JobTemplate` and `EnvironmentTemplate`. `StepParameterSpaceDefinition` had no such registration, so a task parameter spelled `type: int` was rejected with `Input tag 'int' found using 'type' does not match any of the expected tags` even with EXPR declared. The conformance fixture pinning this is `EXPR/job_templates/proposed/3.4.1--task-param-type-case-insensitive.yaml`, added by openjd-specifications#166, which failed here and in openjd-rs alike. The fix is the same three-line registration the other two models carry, on `taskParameterDefinitions`, before discriminated-union resolution. Nothing else was needed: `TaskParameterList` is a list of dicts, the shape the shared function was already written for, and `context.extensions` holds the effective set by then because `JobTemplate` declares `extensions` before `steps`. That covers all five task parameter types, `CHUNK[INT]` included -- §3.4.1.5 makes it a task parameter type name, so EXPR makes `chunk[int]` a spelling of it. The fold now uses an ASCII-only translation table instead of `str.upper()`. `str.upper()` is Unicode-aware and folds ten codepoints wholly into the type-name alphabet, five of which spell a real type: `ıNT` (U+0131), `ſTRING` (U+017F), `flOAT` (U+FB02) and `ſtRING`/`stRING` (U+FB05/U+FB06) were all accepted under EXPR. Spec type names are ASCII, so none of those is a spelling variant. This is a second defect rather than part of the fixture's, and no fixture pins it, but it shares the line being edited -- and registering the fold on task parameters would otherwise have extended it to a surface that was accidentally safe by not folding at all. 81 tests across three files, one class per registration site, each covering the four cases of EXPR on/off by canonical/mis-cased spelling. Nine mutations, all caught: un-registering each of the three sites, dropping `mode="before"`, dropping the EXPR gate, reverting to `str.upper()`, neutering the translation table, removing the two isinstance guards, and folding only the first list element. Three findings came from auditing the tests rather than the code, and each added tests that were missing. The `EnvironmentTemplate` registration was covered by nothing: neutering it left the entire model_v0 suite green at 2743 passed, and the ASCII change had silently altered that shipped path in both directions with no test either way. The two isinstance guards in the shared function became load-bearing on a new surface, since registering the fold routed `taskParameterDefinitions` through them for the first time; without them `taskParameterDefinitions:` left empty raises `TypeError` and `type: 3` raises `AttributeError` out of `decode_job_template` instead of `DecodeValidationError`. And because a mis-cased name now resolves to a variant, it reaches validators it never used to: two mis-cased `chunk[int]` parameters now report the one-CHUNK[INT] rule, and `int`/`INT` under one name now reports the duplicate-name rule. One test was renamed after mutation testing showed it did not pin what its name claimed. A template declaring EXPR to a caller that does not allowlist it is rejected on the extension name, before the type name is reached, so it survives removal of the EXPR gate; it now says so and asserts the extension message. The matching openjd-rs change is not yet raised. Both fixtures stay in `proposed/` until both implementations release, since a fixture is promotable only when it passes on both. Design note: SuperDaveDocs docs/conformance-0901/4.3-parameter-insensitive/fix.md Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
commented
Sep 3, 2026
| @field_validator("taskParameterDefinitions", mode="before") | ||
| @classmethod | ||
| def _normalize_parameter_type_case(cls, v: Any, info: ValidationInfo) -> Any: | ||
| return _normalize_parameter_type_case(v, info) |
Contributor
Author
There was a problem hiding this comment.
Just applies the validator below to support mixed case.
leongdl
added a commit
to OpenJobDescription/openjd-rs
that referenced
this pull request
Sep 5, 2026
Template Schemas §2 says "job parameter and task parameter type names become case-insensitive" when the EXPR extension is enabled, and so are case-sensitive without it. openjd-rs got both halves wrong, in opposite directions and on different parameter kinds. `JobParameterDefinition`'s hand-written `Deserialize` upper-cased the `type` tag unconditionally, so `type: string` was accepted on a base template with no extensions. `TaskParameterDefinition` used the derived internally-tagged impl with exact variant names and never folded, so `type: int` was rejected even with EXPR, reporting `unknown variant 'int', expected one of 'INT', 'FLOAT', 'STRING', 'PATH', 'CHUNK[INT]'`. The one combination that worked -- job parameters under EXPR -- worked by accident, because an ungated fold happens to agree with the spec there. Two conformance fixtures pin one half each: `base/job_templates/proposed/2--type-lowercase.invalid.yaml` from openjd-specifications#163 and `EXPR/job_templates/proposed/3.4.1--task-param-type-case-insensitive.yaml` from #166. Both failed here. Measured by direct path, before and after: #166's goes from invalid to valid, #163's from valid to invalid, and the already-gating `EXPR/job_templates/2--type-case-insensitive.yaml` stays valid. Suite totals do not move, because a `proposed/` fixture is never picked up by directory discovery: 1161 passed and the same 1 pre-existing failure (`3.5--env-script-onexit-only.invalid.yaml`, unrelated and failing on origin/main too) before and after. Two changes, and neither is useful alone. First, `TaskParameterDefinition` gets a hand-written `Deserialize` mirroring `JobParameterDefinition`'s, so both kinds match the tag case-blind. That alone makes the EXPR half pass and the base half worse. Second, a check in `parse.rs` rejects a spelling that names a real type but is not the spec's spelling, when EXPR is not in effect. The check cannot live in `Deserialize`, and that shapes the whole change. The effective extension set is the template's declared list intersected with the caller allowlist, and it is not computed until after `serde_json::from_value` has consumed the document -- while the author's spelling is available only during deserialization, since folding destroys it. So a walk over the raw `serde_json::Value` collects `(error path, type name as written)` for the two places a parameter type name can appear, before the document is moved, and the check consumes that list once `validate_extensions_list` has produced the real extension set. Errors join the same batch, so a template with both an extension problem and a casing problem reports both. The alternatives were a thread-local carrying the EXPR bit into the deserializers, which makes the public `JobTemplate: Deserialize` depend on ambient state, and `DeserializeSeed`, which would mean hand-writing seeds for every type from `JobTemplate` down to `TaskParameterDefinition` and giving up `rename_all` and `deny_unknown_fields`. The cost of the walk is that it knows where type names live independently of the structs; a future revision adding a third location must update it. `decode_environment_template` gets the same wiring. An environment template's `parameterDefinitions` is the same `JobParameterDefinition` union, so a fix applied only to `decode_job_template` would leave it accepting `type: string` in base. Auditing the equivalent Python change found that surface covered by nothing, which is why it has explicit tests here. Both folds are now `to_ascii_uppercase` rather than `to_uppercase`. Unicode folding maps four codepoints wholly into the type-name alphabet -- U+0131 dotless i to I, U+017F long s to S, U+FB02 fl to FL, U+FB06 st to ST -- so `ıNT` was accepted as INT with and without EXPR. Spec type names are ASCII, so none of those is a spelling variant. No fixture pins this, but it shares the line being edited, and making task parameters fold at all would otherwise have extended the defect to a surface that was accidentally safe by not folding. `type: list[int]` without EXPR changes message. It reported `parameter type 'LIST[INT]' is not allowed.` -- naming a canonical spelling for a template that never used it, because `structure.rs` formats `type_name()` -- and now reports the casing, naming what the author wrote. A canonically-spelled EXPR-only type without EXPR still reports `is not allowed.`, which is what eight tests in `test_expr_parameters.rs` assert. An unrecognized task type now reports `unknown task parameter type: 'NOPE'` instead of serde's variant list. 27 tests in `tests/integration/test_param_type_name_case.rs`, covering all four combinations of extension state by spelling on all three surfaces, plus the ASCII fold, the collect-all behaviour, per-step paths, and malformed input. Eleven mutations, all caught: matching the task tag exactly again, reverting either fold to Unicode, disabling the check, dropping its EXPR gate, gating it on any extension instead, dropping either half of the walk, skipping environment templates, walking only the first step, and collecting only the first definition. Design note: SuperDaveDocs docs/conformance-0901/4.3-parameter-insensitive/fix.md The matching openjd-model-for-python change is OpenJobDescription/openjd-model-for-python#350. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
added a commit
to OpenJobDescription/openjd-rs
that referenced
this pull request
Sep 5, 2026
* fix(model): Gate parameter type-name case on the EXPR extension Template Schemas §2 says "job parameter and task parameter type names become case-insensitive" when the EXPR extension is enabled, and so are case-sensitive without it. openjd-rs got both halves wrong, in opposite directions and on different parameter kinds. `JobParameterDefinition`'s hand-written `Deserialize` upper-cased the `type` tag unconditionally, so `type: string` was accepted on a base template with no extensions. `TaskParameterDefinition` used the derived internally-tagged impl with exact variant names and never folded, so `type: int` was rejected even with EXPR, reporting `unknown variant 'int', expected one of 'INT', 'FLOAT', 'STRING', 'PATH', 'CHUNK[INT]'`. The one combination that worked -- job parameters under EXPR -- worked by accident, because an ungated fold happens to agree with the spec there. Two conformance fixtures pin one half each: `base/job_templates/proposed/2--type-lowercase.invalid.yaml` from openjd-specifications#163 and `EXPR/job_templates/proposed/3.4.1--task-param-type-case-insensitive.yaml` from #166. Both failed here. Measured by direct path, before and after: #166's goes from invalid to valid, #163's from valid to invalid, and the already-gating `EXPR/job_templates/2--type-case-insensitive.yaml` stays valid. Suite totals do not move, because a `proposed/` fixture is never picked up by directory discovery: 1161 passed and the same 1 pre-existing failure (`3.5--env-script-onexit-only.invalid.yaml`, unrelated and failing on origin/main too) before and after. Two changes, and neither is useful alone. First, `TaskParameterDefinition` gets a hand-written `Deserialize` mirroring `JobParameterDefinition`'s, so both kinds match the tag case-blind. That alone makes the EXPR half pass and the base half worse. Second, a check in `parse.rs` rejects a spelling that names a real type but is not the spec's spelling, when EXPR is not in effect. The check cannot live in `Deserialize`, and that shapes the whole change. The effective extension set is the template's declared list intersected with the caller allowlist, and it is not computed until after `serde_json::from_value` has consumed the document -- while the author's spelling is available only during deserialization, since folding destroys it. So a walk over the raw `serde_json::Value` collects `(error path, type name as written)` for the two places a parameter type name can appear, before the document is moved, and the check consumes that list once `validate_extensions_list` has produced the real extension set. Errors join the same batch, so a template with both an extension problem and a casing problem reports both. The alternatives were a thread-local carrying the EXPR bit into the deserializers, which makes the public `JobTemplate: Deserialize` depend on ambient state, and `DeserializeSeed`, which would mean hand-writing seeds for every type from `JobTemplate` down to `TaskParameterDefinition` and giving up `rename_all` and `deny_unknown_fields`. The cost of the walk is that it knows where type names live independently of the structs; a future revision adding a third location must update it. `decode_environment_template` gets the same wiring. An environment template's `parameterDefinitions` is the same `JobParameterDefinition` union, so a fix applied only to `decode_job_template` would leave it accepting `type: string` in base. Auditing the equivalent Python change found that surface covered by nothing, which is why it has explicit tests here. Both folds are now `to_ascii_uppercase` rather than `to_uppercase`. Unicode folding maps four codepoints wholly into the type-name alphabet -- U+0131 dotless i to I, U+017F long s to S, U+FB02 fl to FL, U+FB06 st to ST -- so `ıNT` was accepted as INT with and without EXPR. Spec type names are ASCII, so none of those is a spelling variant. No fixture pins this, but it shares the line being edited, and making task parameters fold at all would otherwise have extended the defect to a surface that was accidentally safe by not folding. `type: list[int]` without EXPR changes message. It reported `parameter type 'LIST[INT]' is not allowed.` -- naming a canonical spelling for a template that never used it, because `structure.rs` formats `type_name()` -- and now reports the casing, naming what the author wrote. A canonically-spelled EXPR-only type without EXPR still reports `is not allowed.`, which is what eight tests in `test_expr_parameters.rs` assert. An unrecognized task type now reports `unknown task parameter type: 'NOPE'` instead of serde's variant list. 27 tests in `tests/integration/test_param_type_name_case.rs`, covering all four combinations of extension state by spelling on all three surfaces, plus the ASCII fold, the collect-all behaviour, per-step paths, and malformed input. Eleven mutations, all caught: matching the task tag exactly again, reverting either fold to Unicode, disabling the check, dropping its EXPR gate, gating it on any extension instead, dropping either half of the walk, skipping environment templates, walking only the first step, and collecting only the first definition. Design note: SuperDaveDocs docs/conformance-0901/4.3-parameter-insensitive/fix.md The matching openjd-model-for-python change is OpenJobDescription/openjd-model-for-python#350. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> * refactor(model): Give the type-tag fold one home The two parameter deserializers each read the `type` tag, folded it, and stripped it before matching -- twelve duplicated lines carrying the rule this change set exists to fix. Two copies of a folding rule is how the job and task halves came to disagree about what a spelling variant is in the first place, so the shared prologue moves into `split_type_tag`, which returns the tag as written, the tag folded for matching, and the body. Three consequences, all wanted. The ASCII fold now exists once, so it cannot drift again. `strip_type_field` goes back to module-private, since the new helper is the intended shared entry point and is the only thing that needed widening. And the mutation that reverts the fold to `to_uppercase` now fails all four non-ASCII lookalike groups rather than two each, because one site covers both kinds. Also spells `PathElement` through the file's own import rather than `crate::error::PathElement` inline, and builds the root path with `vec![]` to match `limits.rs:14` and `structure.rs:25`. No behaviour change. `openjd-model` stays at 1984 passed, all eleven mutations stay caught, and clippy, rustfmt and `cargo doc` stay clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --------- Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Template Schemas §2 says "job parameter and task parameter type names become case-insensitive" when the EXPR extension is enabled, and so are case-sensitive without it.
_normalize_parameter_type_casealready implemented that, gated on the extension, but it was registered only onparameterDefinitionsinJobTemplateandEnvironmentTemplate.StepParameterSpaceDefinitionhad no such registration, so a task parameter spelledtype: intwas rejected even with EXPR declared:The fix is the same registration the other two models carry, on
taskParameterDefinitions, before discriminated-union resolution. Nothing else was needed:TaskParameterListis a list of dicts, the shape the shared function was already written for, andcontext.extensionsholds the effective set by then becauseJobTemplatedeclaresextensionsbeforesteps. That covers all five task parameter types,CHUNK[INT]included — §3.4.1.5 makes it a task parameter type name, so EXPR makeschunk[int]a spelling of it.The fold now uses an ASCII-only translation table instead of
str.upper().str.upper()is Unicode-aware and folds ten codepoints wholly into the type-name alphabet, five of which spell a real type:ıNT(U+0131),ſTRING(U+017F),flOAT(U+FB02) andſtRING/stRING(U+FB05/U+FB06) were all accepted under EXPR. Spec type names are ASCII, so none of those is a spelling variant. This is a second defect rather than part of the fixture's, and no fixture pins it, but it shares the line being edited — and registering the fold on task parameters would otherwise have extended it to a surface that was accidentally safe by not folding at all.Conformance
The fixture is
EXPR/job_templates/proposed/3.4.1--task-param-type-case-insensitive.yaml, added by OpenJobDescription/openjd-specifications#166, which failed here and in openjd-rs alike. Both arejob_templates/fixtures, so what they assert is template validity — whatopenjd checkgets fromdecode_job_template. Loading the real fixture files and decoding them, before and after this commit:EXPR/…/proposed/3.4.1--task-param-type-case-insensitive.yamlInput tag 'pAtH'base/…/proposed/2--type-lowercase.invalid.yamlEXPR/job_templates/2--type-case-insensitive.yaml(already gating)The fixture this package was failing now passes, the base fixture it already satisfied is unchanged, and the already-green gating fixture stays green. The conformance runner itself was not executed, because that needs openjd-cli and openjd-sessions built against the patched model.
The fixture stays in
proposed/until openjd-rs ships the matching change, since a fixture is promotable only when it passes on both implementations.Tests
81 tests across three classes, one per registration site, each covering the four cases of EXPR on/off by canonical/mis-cased spelling.
TestTaskParameterTypeNameCasetest_parameter_space.pyStepParameterSpaceDefinition.taskParameterDefinitionsTestJobParameterTypeNameCasetest_list_parameters.pyJobTemplate.parameterDefinitionsTestEnvironmentTemplateParameterTypeNameCasetest_environment_template.pyEnvironmentTemplate.parameterDefinitionsNine mutations, each caught by the tests that should catch it:
taskParameterDefinitionstest_with_expr_miscased_accepted×5 + 3mode="before"JobTemplatetest_with_expr_miscased_accepted×4EnvironmentTemplatetest_with_expr_miscased_accepted×3test_no_expr_miscased_rejected×12 + 2str.upper()test_non_ascii_lookalike_rejected_with_expr×11isinstanceguardstest_malformed_input_is_a_validation_error_not_a_crash×6Two groups survive every mutation and are negative controls rather than false pins.
test_no_expr_canonical_case_acceptedproves a mis-cased rejection is about the spelling and not about the type being unavailable.test_non_ascii_lookalike_rejected_without_expris inert by construction, since without EXPR no fold runs; it exists so the pair covers both extension states, and its comment says so.Full suite: 5626 passed, against a 5545 baseline. The 12 failures are pre-existing and unrelated — all in
test/openjd/model_v1/test_step_param_space_iter.py::TestChunksTaskCountOverride, one cause, a stale Rust crate pin in the v1 binding layer. Byte-identical failure set before and after.ruff,blackandmypyclean.What test auditing found that the design missed
Three findings came from auditing the tests rather than the code, and each added tests that were missing.
The
EnvironmentTemplateregistration was covered by nothing. Neutering it left the entiremodel_v0suite green at 2743 passed, where neutering theJobTemplateone fails 6 tests. The ASCII change had therefore silently altered that shipped path in both directions with no test either way.The shared function's two
isinstanceguards became load-bearing on a new surface. Registering the fold routedtaskParameterDefinitionsthrough them for the first time. Measured with the guards removed:taskParameterDefinitions:left empty raisesTypeError: 'NoneType' object is not iterable,type: 3raisesAttributeError: 'int' object has no attribute 'translate', and a missingtyperaisesKeyError, each escapingdecode_job_templateinstead of arriving as aDecodeValidationError.A mis-cased name now reaches validators it never used to. Before the fold ran on this field,
type: intfailed at discriminator resolution and nothing downstream saw it. Now it resolves, so two mis-casedchunk[int]parameters report the one-CHUNK[INT]-per-step rule andint/INTunder one name reports the duplicate-name rule. Both are correct and both are new, so both are asserted.One test was renamed because mutation testing showed it did not pin what its name claimed. A template declaring EXPR to a caller that does not allowlist it is rejected on the extension name, before the type name is reached, so it survives removal of the EXPR gate. It now says so and asserts the extension message.
Out of scope
Pydantic's wording for a type name that is wrong for reasons other than case (
Input tag 'NOPE' found using 'type' does not match any of the expected tags) is unhelpful but is not this defect, and is not worth diverging from the framework to improve.openjd.model._v1is entirely re-exports from theopenjd._openjd_rsextension module, so that surface is fixed by the openjd-rs change rather than here.Related
SuperDaveDocs docs/conformance-0901/4.3-parameter-insensitive/fix.md