fix(model): Gate parameter type-name case on the EXPR extension - #364
Merged
Conversation
mwiebe
approved these changes
Sep 3, 2026
wyongzhi
approved these changes
Sep 3, 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>
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>
leongdl
force-pushed
the
fix/param-type-case-expr-gated
branch
from
September 5, 2026 00:37
aac8fac to
0538183
Compare
leongdl
commented
Sep 5, 2026
Open
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
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. openjd-rs got both halves wrong, in opposite directions and on different parameter kinds.
JobParameterDefinition's hand-writtenDeserializeupper-cased thetypetag unconditionally, sotype: stringwas accepted on a base template with no extensions.TaskParameterDefinitionused the derived internally-tagged impl with exact variant names and never folded, sotype: intwas rejected even with EXPR:The one combination that worked — job parameters under EXPR — worked by accident, because an ungated fold happens to agree with the spec there.
Conformance
Two fixtures pin one half each:
base/job_templates/proposed/2--type-lowercase.invalid.yamlfrom OpenJobDescription/openjd-specifications#163, andEXPR/job_templates/proposed/3.4.1--task-param-type-case-insensitive.yamlfrom #166. Both failed here. Measured by direct path against release builds oforigin/mainand this branch:EXPR/…/proposed/3.4.1--task-param-type-case-insensitive.yamlbase/…/proposed/2--type-lowercase.invalid.yamlEXPR/job_templates/2--type-case-insensitive.yaml(already gating)Suite totals do not move, because a
proposed/fixture is never picked up by directory discovery. Full2023-09/*run: 1161 passed, 1 failed, before and after. That one failure isbase/job_templates/3.5--env-script-onexit-only.invalid.yaml, unrelated to this change and failing onorigin/maintoo.The two changes
Neither is useful alone.
TaskParameterDefinitiongets a hand-writtenDeserializemirroringJobParameterDefinition's, so both kinds match the tag case-blind. That alone makes the EXPR half pass and the base half worse.A check in
parse.rsrejects a spelling that names a real type but is not the spec's spelling, when EXPR is not in effect. A spelling that names no type at all is left to deserialization, so an unknown type reports once rather than twice.Why the check is not in
DeserializeThis is the part of the change most open to a reasonable objection, so here is the argument.
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_valuehas consumed the document. The author's spelling is available only during deserialization, since folding destroys it. So a walk over the rawserde_json::Valuecollects(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 oncevalidate_extensions_listhas produced the real extension set. Errors join the same batch, so a template with both an extension problem and a casing problem reports both.Two alternatives were rejected. A thread-local carrying the EXPR bit into the deserializers is the smallest diff, but
JobTemplate: Deserializeis public, so any caller doingserde_json::from_value::<JobTemplate>outsidedecode_job_templatewould get whichever behaviour the ambient default happened to be — and it would re-derive the extension set from the template body without the caller allowlist, which is the wrong set.DeserializeSeedis the principled mechanism, but every intermediate type fromJobTemplatedown toTaskParameterDefinitionis a derive today, and hand-writing seeds for them gives uprename_allanddeny_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, and nothing will fail if it is not updated. That is the trade recorded in the design note.
decode_environment_templategets the same wiring. An environment template'sparameterDefinitionsis the sameJobParameterDefinitionunion, so a fix applied only todecode_job_templatewould leave it acceptingtype: stringin base. Auditing the equivalent Python change found that surface covered by nothing, which is why it has explicit tests here.The ASCII fold
Both folds are now
to_ascii_uppercaserather thanto_uppercase. Unicode folding maps four codepoints wholly into the type-name alphabet — U+0131 dotless i toI, U+017F long s toS, U+FB02fltoFL, U+FB06sttoST— soıNTwas accepted asINTwith and without EXPR. Spec type names are ASCII, so none of those is a spelling variant.No fixture pins this, and it is a second defect rather than part of the fixtures'. It is here because it shares the line being edited, and because making task parameters fold at all would otherwise have extended the defect to a surface that was accidentally safe by not folding.
Message changes
type: list[int]without EXPR reportedparameter type 'LIST[INT]' is not allowed.— naming a canonical spelling for a template that never used it, becausestructure.rsformatstype_name(). It now reports the casing, naming what the author wrote. A canonically-spelled EXPR-only type without EXPR still reportsis not allowed., which is what eight tests intest_expr_parameters.rsassert and why the check deliberately skips a name it recognizes as canonical.An unrecognized task parameter type now reports
unknown task parameter type: 'NOPE'instead of serde's derived variant list, matching the job-parameter wording.Tests
27 tests in
tests/integration/test_param_type_name_case.rs. All three surfaces a parameter type name can appear on are covered in all four combinations of extension state by spelling, plus the ASCII fold, collect-all behaviour, per-step paths, and malformed input. They live in one file because one check governs all three surfaces.Eleven mutations, all caught by the tests that should catch them:
test_task_with_expr_miscased_accepted+ 5to_uppercasetest_job_non_ascii_lookalike_*×2to_uppercasetest_task_non_ascii_lookalike_*×2*_with_expr_miscased_acceptedtest_task_chunk_int_miscased_needs_expr_not_only_task_chunkingtest_env_no_expr_miscased_rejectedtest_miscased_task_type_on_a_later_step_reports_that_steps_pathtest_all_miscased_names_are_reportedcargo test --workspaceall green, 1984 inopenjd-modelagainst a 1957 baseline.cargo clippy --all-features --all-targets --workspace -- -D warningsandcargo fmt --all -- --checkclean.Also replaced a stale comment in
test_expr_parameters.rsthat said the Python case-insensitivity tests were not ported "because the Rust implementation accepts case-insensitive types by default" — no longer true, and it now points at the new file.Out of scope
JobParameterType::from_spec_strandTaskParameterType::from_spec_strkeep their unconditional case-insensitivity. They are public and had no non-test caller; the new check is their first, and it needs the permissive reading for the canonical round-trip.specs/model/public-api.mdannotates only the job one as case-insensitive, so that document implies the two differ when they do not — worth a follow-up.Other case-sensitive discriminants stay case-sensitive:
dataFlow,objectType,userInterface.control,rangeConstraint, the embedded-filetype. §2's sentence names parameter type names only.Related
SuperDaveDocs docs/conformance-0901/4.3-parameter-insensitive/fix.mdproposed/until both implementations release, since a fixture is promotable only when it passes on both.