Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions THIRD-PARTY-LICENSES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2535,8 +2535,8 @@ limitations under the License.
** libc; version 0.2.189 -- https://crates.io/crates/libc
** manyhow-macros; version 0.11.4 -- https://crates.io/crates/manyhow-macros
** openjd-expr; version 0.6.0 -- https://crates.io/crates/openjd-expr
** openjd-model; version 0.6.0 -- https://crates.io/crates/openjd-model
** openjd-sessions; version 0.5.5 -- https://crates.io/crates/openjd-sessions
** openjd-model; version 0.6.1 -- https://crates.io/crates/openjd-model

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 regenerated file drops two license sections that have nothing to do with this version bump:

  • ** wasi; version 0.11.1+wasi-snapshot-preview1 under the Apache-2.0 WITH LLVM-exception text (old lines 819–1049) — the --- LLVM Exceptions to the Apache 2.0 License ---- block is now gone from the file entirely.
  • ** encoding_rs; version 0.8.35 under the BSD-3-Clause text that was harvested from the crate’s source file (the //-commented variant with the GB18030_2022_OVERRIDE_PUA table, old lines 2639–2699).

Neither crate changed: both are still in Cargo.lock at the same versions (encoding_rs 0.8.35 via encoding_rs_ioserde-saphyr, wasi 0.11.1 via getrandom 0.2.17/mio), and this diff only touches the openjd-model/openjd-sessions entries. Attribution is not lost (wasi still appears in the plain Apache-2.0 group, encoding_rs in the Apache-2.0 and clean BSD-3-Clause groups), so this looks like cargo about picking a different license variant / license-file source than it did for the committed baseline — i.e. a cargo-about version difference on the machine that ran --update, not a dependency change.

That matters because nothing pins the tool: scripts/check_third_party_licenses.sh just requires cargo-about on PATH, and .github/workflows/rust_quality.yml:204 runs cargo install cargo-about --locked --features cli, which resolves to whatever the latest published version is at job time. --locked pins cargo-about’s own dependencies, not cargo-about itself, so the generated output can drift between the contributor’s machine and CI, and between CI runs over time — producing spurious THIRD-PARTY-LICENSES check failures on diffs that never touched dependencies.

Two things worth confirming before merge:

  1. That these two deletions are intended and reviewed as a licensing change, rather than incidental fallout — dropping the LLVM-exception text is fine only because wasi is also offered under plain Apache-2.0.
  2. Consider pinning the tool (cargo install cargo-about --locked --features cli --version X.Y.Z) in both the workflow and the script’s doc comment so regeneration is reproducible.

** openjd-sessions; version 0.5.6 -- https://crates.io/crates/openjd-sessions
** pin-project-lite; version 0.2.17 -- https://crates.io/crates/pin-project-lite
** portable-atomic; version 1.15.0 -- https://crates.io/crates/portable-atomic
** proc-macro2; version 1.0.107 -- https://crates.io/crates/proc-macro2
Expand Down
4 changes: 2 additions & 2 deletions rust-bindings/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ crate-type = ["cdylib", "rlib"]

[dependencies]
openjd-expr = "0.6.0"
openjd-model = "0.6.0"
openjd-sessions = "0.5.5"
openjd-model = "0.6.1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

THIRD-PARTY-LICENSES.txt was not regenerated for this bump. It still records the old versions:

2538:** openjd-model; version 0.6.0 -- https://crates.io/crates/openjd-model
2539:** openjd-sessions; version 0.5.5 -- https://crates.io/crates/openjd-sessions

cargo about renders that section from the workspace Cargo.lock, so the THIRD-PARTY-LICENSES check job in .github/workflows/rust_quality.yml (which runs scripts/check_third_party_licenses.sh) should fail on this diff. Every previous crate bump in this repo updated the file alongside Cargo.lock (e.g. 7222ce3, c6f7805).

Running ./scripts/check_third_party_licenses.sh --update and committing the result should resolve it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This bump pulls in a model-layer validation change that the pure-Python v0 reference does not have, so it silently creates a v0/v1 divergence.

openjd-model 0.6.1 includes openjd-rs#360, whose model half "reject[s] NUL in template-declared environment variable values so openjd check catches it at validation time" (structure.rs). After this bump, decode_job_template through the binding rejects a template whose environments[].variables value contains a YAML-escaped NUL.

The v0 model still accepts it. EnvironmentVariableValueString (src/openjd/model/v2023_09/_model.py:1954) only caps length at 2048, and _validate_variables (:2023) only rejects an empty map -- there is no NUL check anywhere in src/. Grepping for the escaped NUL codepoint across src/ finds only two unrelated hits, in _variable_reference_validation.py and _openjd_rs.pyi.

So the same template now validates under openjd.model v0 and fails under openjd.model._v1. That is exactly the class of thing AGENTS.md:184 (reference parity) and AGENTS.md:188 (known gaps) ask to be tracked, and there is no coverage on either side: no test in test/openjd/model_v1/ exercising a NUL in an environment variable value, and nothing in test/openjd/model_v1/test_known_gaps.py recording the gap. The v0 suite does test NUL rejection for other string types (test/openjd/model_v0/v2023_09/test_strings.py:156 and four other sites), so the omission here reads as unported rather than deliberate.

Note the contrast with the other half of this bump: openjd-rs#364 (EXPR type-name case gating) was ported to Python first, in #350 -- which is this PR's base commit. #360 appears to have no Python counterpart, so the bump lands the Rust half unaccompanied.

Worth deciding which way to close it before merge: port the NUL rejection to the v0 model (matching what #350 did for the case fix), or add an xfail in test_known_gaps.py recording the divergence. Either way a v1 test asserting the new ModelValidationError message and field path would pin the binding's behaviour, per AGENTS.md:178.

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 v1 parity tests for the EXPR type-name-case rule belong in this PR, and are missing.

openjd-model 0.6.1 is the release that carries openjd-rs#364, so this bump is the commit where the binding's behaviour actually changes on three surfaces: job parameterDefinitions, step taskParameterDefinitions, and environment-template parameterDefinitions. Before it, the binding accepted type: string on a base template with no extensions and rejected type: int even with EXPR; after it, both are gated correctly.

The base commit (#350) landed the matching Python change plus 447 lines of v0 tests:

  • test/openjd/model_v0/v2023_09/test_parameter_space.py:779 TestTaskParameterTypeNameCase -- all four spelling/extension combinations across all five task parameter types
  • test/openjd/model_v0/v2023_09/test_list_parameters.py:115 -- the same four cases for job parameters
  • test/openjd/model_v0/v2023_09/test_environment_template.py -- the environment-template surface

There is no v1 counterpart for any of them. Grepping test/openjd/model_v1/ for miscased|case-insensitive|case_insensitive|type_name_case returns nothing, and test_known_gaps.py has no entry either. AGENTS.md:184 asks for a v1 equivalent for every v0 reference test; the reason the v1 side was legitimately empty at #350 was that the binding did not yet implement the rule. This PR removes that reason.

That matters more than usual here because the two implementations were wrong in opposite directions on different parameter kinds, and #364 also tightened the fold from to_uppercase to to_ascii_uppercase (so INT spelled with U+0131 dotless-i is no longer accepted). Nothing in this repo currently pins any of that through the binding, so a future regression on either side -- or a drift in the error text, which AGENTS.md:178 asks tests to assert -- would go unnoticed.

Mirroring the three v0 classes into test/openjd/model_v1/, asserting the ModelValidationError message and field path, would close it.

openjd-sessions = "0.5.6"
tokio = { version = "1", features = ["rt-multi-thread"] }
uuid = { version = "1", features = ["v4"] }
serde_json = "1"
Expand Down
Loading