Skip to content

fix(model): Resume deferred validation after format-string resolution - #349

Merged
leongdl merged 3 commits into
OpenJobDescription:mainfrom
leongdl:fix/post-resolution-coercion-checks
Sep 1, 2026
Merged

fix(model): Resume deferred validation after format-string resolution#349
leongdl merged 3 commits into
OpenJobDescription:mainfrom
leongdl:fix/post-resolution-coercion-checks

Conversation

@leongdl

@leongdl leongdl commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

What

Three constraints were correctly deferred past decode because their value was a format string, and then never resumed at job creation. Two commits, one defect shape.

A field annotated @fmtstring can hold a value that is unknown at decode, so a constraint on that value cannot be applied there. Decode does the right thing and skips the check. Job creation resolves the value, so that is where the check belongs. It was missing.

Commit 1: attribute capability values

hostRequirements.attributes[].anyOf and .allOf are @fmtstring in base 2023-09 with no extension gate, and their element type <AttributeCapabilityValue> (§3.3.2.2) constrains the value to an identifier-like pattern with a 100-character limit. validate_v2023_09::structure gates its check on FormatString::is_literal, correctly. Nothing re-checked after resolution, so this ran to completion:

parameterDefinitions:
- name: Software
  type: STRING
  default: "not valid!"
steps:
- name: Step1
  hostRequirements:
    attributes:
    - name: attr.custom.software
      anyOf: ['{{Param.Software}}']

resolve_host_requirements now re-checks each resolved element through a new
capabilities::validate_attribute_capability_value. The error carries the same path and
wording as the decode-time check, so the violation reads identically whether the value was
written literally or arrived through a parameter:

steps[0] -> hostRequirements -> attributes[0] -> anyOf[0]:
	value 'not valid!' contains invalid characters.

The standard-capability branch is exclusive, matching the reference implementation: a standard capability is checked against its value set only, not against the identifier pattern or the length limit.

instantiate_step gained a step index so the path can name the step. That takes it to six parameters, which is over this repo's comfortable limit, and it is there to make the error path identical to decode's.

Commit 2: amount bounds, and chunk counts

hostRequirements.amounts[].min and .max may be format strings under FEATURE_BUNDLE_1. The decode checks read parse_literal_amount, which returns None for a non-literal, so min non-negative, max positive and min <= max were skipped together. Job creation checked only that the resolved text parsed as a finite number. A max resolving to 0 was accepted even though max is <positivefloat> while min is <nonnegativefloat>. check_resolved_amount_bounds re-applies all three.

chunks.defaultTaskCount and .targetRuntimeSeconds were worse than unchecked. Job creation resolved them and then clamped, with .max(1) and .max(0). A defaultTaskCount resolving to 0 silently became 1, and the job ran with a chunk shape its author never asked for — the observed log was five one-task chunks. Both now reject the out-of-range value. The literal arms clamped too, but decode rejects an out-of-range literal, so those clamps were unreachable.

Why it matters

These are the four proposed/ fixtures in openjd-specifications#179, which exist because the Python reference implementation rejects all four and this one accepted all four. With this PR all four pass here as well, so they can be promoted into the live suite once openjd-cli ships from crates.io. The spec repo's Rust conformance job installs the published release, so promoting before then would turn that leg red.

Verification

cargo test --workspace 7338 passed, 0 failed, up 24 from 7314. Full OpenJD conformance suite 1174 passed, 0 failed, unchanged from before either commit. cargo clippy --all-features --all-targets --workspace -- -D warnings clean. cargo fmt --all -- --check clean.

24 tests added. Every one was mutation-checked, which means the fix was reverted one behaviour at a time and the matching test confirmed to fail: 8 mutants on commit 1 and 8 on commit 2, 16 of 16 caught, verified independently by a second auditor that ran its own 14 mutants against commit 1. Two are worth naming. One pins min of 0 as accepted where max of 0 is rejected, so a check written as min <= 0.0 fails it. Another pins the boundary defaultTaskCount of 1 as carried through as 1, so restoring the clamp fails it rather than passing by coincidence.

specs/model/capabilities.md, public-api.md, job-creation.md and validation.md are updated in the same commits, per the repo's spec-and-code rule. validate_attribute_capability_value is new public API and is in public-api.md.

Known remaining gaps, documented not fixed

specs/model/validation.md now states these rather than leaving them implied.

Only the first failing anyOf/allOf group on the first failing attribute is reported, because the caller collects through ?. Decode accumulates every violation. Closing that means threading one ValidationErrors through the whole of resolve_host_requirements.

Decode applies the element length and emptiness checks to the raw text of a format string, not only to literals, so a format string longer than 100 characters is rejected at decode even when it resolves to a short value. Verified: a 130-character element built from two 55-character parameter names resolving to shortshort is rejected with exceeds 100 characters. That is arguably its own defect and is out of scope here.

@leongdl
leongdl requested a review from a team as a code owner August 29, 2026 02:21
Comment thread specs/model/validation.md Outdated
Comment thread crates/openjd-model/src/job/create_job/ranges.rs
Comment thread crates/openjd-model/src/capabilities.rs
Comment thread specs/model/validation.md Outdated
Comment thread crates/openjd-model/src/job/create_job/instantiate.rs
Comment thread crates/openjd-model/src/capabilities.rs
Comment thread crates/openjd-model/src/job/create_job/instantiate.rs
Comment thread crates/openjd-model/src/job/create_job/ranges.rs
@seant-aws

Copy link
Copy Markdown
Contributor

NIT (ranges.rs): The two new chunk-bound errors (defaultTaskCount resolved to < 1, targetRuntimeSeconds resolved to < 0) use bare ModelError::Expression with no field path, while:

  • Decode-time equivalents in validate_v2023_09/task_chunking.rs report full paths like steps[0] -> parameterSpace -> taskParameterDefinitions[0] -> chunks: defaultTaskCount must be >= 1.
  • The other two fixes in this same PR (check_resolved_amount_bounds, check_resolved_attribute_values) both thread step_index and build proper ValidationErrors paths.

Since step_index is already plumbed through instantiate_stepresolve_host_requirements, it would be consistent to thread it into resolve_task_parameter as well and emit path-prefixed errors here too — matching both decode and the rest of this PR.

Not a blocker — the error messages themselves are clear and tested — but it's the one place the diagnostics format diverges from the repo's "assert path + message" standard (AGENTS.md).

@leongdl

leongdl commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

NIT (ranges.rs): The two new chunk-bound errors (defaultTaskCount resolved to < 1, targetRuntimeSeconds resolved to < 0) use bare ModelError::Expression with no field path, while:

  • Decode-time equivalents in validate_v2023_09/task_chunking.rs report full paths like steps[0] -> parameterSpace -> taskParameterDefinitions[0] -> chunks: defaultTaskCount must be >= 1.
  • The other two fixes in this same PR (check_resolved_amount_bounds, check_resolved_attribute_values) both thread step_index and build proper ValidationErrors paths.

Since step_index is already plumbed through instantiate_stepresolve_host_requirements, it would be consistent to thread it into resolve_task_parameter as well and emit path-prefixed errors here too — matching both decode and the rest of this PR.

Not a blocker — the error messages themselves are clear and tested — but it's the one place the diagnostics format diverges from the repo's "assert path + message" standard (AGENTS.md).

I will make a follow up PR to upgrade the error messages. This one can unlock conformance testing.

`hostRequirements.attributes[].anyOf` and `.allOf` are @fmtstring in base
2023-09, so decode cannot apply the `<AttributeCapabilityValue>` constraints
of spec section 3.3.2.2 to an element written as a format string. Decode
correctly gates that check on `FormatString::is_literal`, but nothing resumed
it after resolution, so a template whose parameter resolved to `not valid!`
created a job and ran. The Python reference implementation rejects it at job
creation.

`resolve_host_requirements` now re-checks each resolved element through the
new `capabilities::validate_attribute_capability_value`. Errors are reported
at `steps[i] -> hostRequirements -> attributes[j] -> anyOf[k]`, matching the
decode-time path and wording, so the violation reads the same way whether the
value was a literal or arrived through a parameter. `instantiate_step` gained
a step index to build that path.

The standard-capability branch is exclusive, matching the reference
implementation: a standard capability is checked against its value set only,
not the identifier pattern or the length limit.

Closes the openjd-specifications conformance fixtures
base/jobs/proposed/3.3.2--format-string-in-{anyof,allof}-resolves-to-invalid-value.
Both now pass against both reference implementations.

The sibling deferrals on the same function are unchanged and still diverge
from Python: an amount `min`/`max` that is a format string has only its
finiteness re-checked, not the non-negative, positive and `min <= max`
bounds. specs/model/validation.md now states that explicitly.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Two more constraints were deferred past decode for a format string and never
resumed, the same shape as the attribute capability values fixed in the
previous commit.

`hostRequirements.amounts[].min` and `.max` may be format strings under
FEATURE_BUNDLE_1. The decode-time checks read `parse_literal_amount`, which
returns `None` for a non-literal, so `min` non-negative, `max` positive and
`min <= max` were all skipped. Job creation checked only that the resolved text
parsed as a finite number, so a `max` resolving to `0` was accepted even though
`max` is `<positivefloat>` and `min` is `<nonnegativefloat>`. The new
`check_resolved_amount_bounds` re-applies all three at the same paths and with
the same wording decode uses.

`chunks.defaultTaskCount` and `.targetRuntimeSeconds` were worse than
unchecked. Job creation resolved them and then clamped, with `.max(1)` and
`.max(0)`, so a `defaultTaskCount` resolving to `0` silently became `1` and the
job ran with a chunk shape its author never asked for. Both now reject the
out-of-range value. The literal arms clamped too, but decode rejects a literal
out-of-range value, so those clamps were unreachable.

Closes the remaining two openjd-specifications conformance fixtures under
`proposed/`: TASK_CHUNKING default-task-count-format-string-resolves-to-zero
and FEATURE_BUNDLE_1 3.3.1--amount-max-format-string-resolves-to-zero. With
this commit all four `proposed/` post-resolution fixtures pass, and the Python
reference implementation already passed all four.

10 tests added, each mutation-checked: 8 mutants, 8 caught, including one that
proves `min` of 0 is accepted where `max` of 0 is not.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
…g a literal

Decode measured the raw text of a format string against the 100-character
<AttributeCapabilityValue> limit, and against the emptiness check, while the
pattern check on the next line was already gated on is_literal. So a 176-character
format string resolving to a legal 14-character value was rejected at decode. The
reference implementation accepts it: _validate_attribute_list skips any
FormatString carrying expressions, and AttributeCapabilityValue declares no
maximum length. Both checks are re-applied to the resolved value at job creation,
so gating loses nothing.

Also updates the two spec paragraphs the previous commit made false: the amount
bounds and the chunk minimums ARE re-applied after resolution now.

cargo test --workspace 7348 passed. Conformance 1174 passed 0 failed.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
@leongdl
leongdl force-pushed the fix/post-resolution-coercion-checks branch from 355e553 to 1b512d9 Compare September 1, 2026 00:13
@leongdl
leongdl enabled auto-merge (squash) September 1, 2026 00:13
@leongdl
leongdl merged commit 73db60a into OpenJobDescription:main Sep 1, 2026
22 checks passed
@leongdl
leongdl deleted the fix/post-resolution-coercion-checks branch September 1, 2026 00:43
@github-actions github-actions Bot mentioned this pull request Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants