Conversation
…authored error paths
Template validation only validated `step.script`; a SimpleAction step
(`bash:`/`python:`/`cmd:`/`powershell:`/`node:`, FEATURE_BUNDLE_1 §8)
was never checked beyond its `let` bindings, while `create_job` checked
the desugared form and reported paths like
`steps[0] -> script -> embeddedFiles[0] -> data` — a node the author
never wrote. So a 500-char `bash:` body under `max_resolved_data_len:
100` passed `check` and failed at submission, and undefined `Param.*`
references, `timeout: 0`, `notifyPeriodInSeconds: 700`, an empty
`script`, and a `let` without EXPR all passed `check`.
Passes 6 and 8 now desugar each SimpleAction field
(`SimpleActionKind::desugar`) and run it through the same helpers an
authored `script` gets (`validate_step_script`,
`validate_step_script_format_strings`), so the stages agree by
construction. Every diagnostic — at decode and at `create_job` — is
re-rooted onto the field the author wrote through one remap table
(`SimpleActionKind::remap_desugared_path`, applied by the crate-private
`ValidationErrors::extend_remapped`): `embeddedFiles[0] -> data` maps
to `<kind> -> script`, `args[k]` to `args[k - offset]` with the
interpreter prefix and generated file reference subtracted,
`timeout`/`cancelation`/`let[j]` pass through, and anything synthesized
collapses to the `<kind>` field itself. Sugar is skipped when the step
also has `script` (pass 7 reports the conflict; validating against a
symtab seeded from the authored script would name the generated file).
`SimpleAction.script` is now a `FormatString` — the spec types it
`<DataString>` `@fmtstring[host]`, and the Python reference agrees — so
malformed brace syntax fails at parse time instead of inside
`create_job` with no path, and `resolve_syntax_sugar` is infallible.
The step-name sanitizer now keeps only `[A-Za-z0-9]`, matching the
Python reference; non-ASCII alphanumerics (`²x`, `a½`) used to panic at
the generated `{{Task.File.<name>}}` reference.
The comprehension-variable check now also covers embedded-file `data`
for authored scripts, since the sugar's `script` desugars to it.
Pinned by 28 tests in `test_simple_action_validation.rs` asserting full
path + message at both stages, including that no diagnostic for a sugar
step names the desugared form. Conformance 1139/1139.
BREAKING CHANGE: `template::SimpleAction.script` is `FormatString`
instead of `String`; read it with `.raw()`.
`StepTemplate::resolve_syntax_sugar` returns `Option<StepScript>`
instead of `Result<Option<StepScript>, ModelError>`. New public
`template::SimpleActionKind` and `StepTemplate::simple_actions` /
`simple_action`.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
mwiebe
force-pushed
the
fix/simple-action-desugar-validation
branch
from
October 1, 2026 00:05
5f0a91d to
2147dc9
Compare
mwiebe
enabled auto-merge (squash)
October 1, 2026 02:31
This branch has not been deployed
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 was the problem/requirement? (What/Why)
Open Job Description templates can define a step's work in two ways. The full form is a
scriptblock with a command, arguments, and embedded files. The short form (a "SimpleAction", from the FEATURE_BUNDLE_1 extension) is abash:,python:,cmd:,powershell:, ornode:block containing just the script text and optional arguments. Internally the short form is expanded ("desugared") into the full form before a job runs.The two forms were not validated the same way:
openjd checkvalidated only the full form. A short-form step was barely checked: undefined variables, atimeoutof 0, an empty script, or a script body over an opted-in size cap all passedcheck.steps[0] -> script -> embeddedFiles[0] -> datafor a step that has noscript.{{ ... }}in a short-form script was only caught at job creation, with no field path at all.What was the solution? (How)
Template validation now expands each short-form step and runs it through the same validation code as a full-form step, so both forms are held to the same rules by construction. Every error produced this way is then mapped back to the field the author actually wrote:
embeddedFiles[0] -> databecomesbash -> script, argument indexes have the generated interpreter arguments subtracted soargs[1]means the author's second argument, and anything synthesized (the generated command or file name) points at thebash:field itself. Job creation uses the same mapping.Two smaller changes came with this. The short form's
scriptfield is now parsed as a format string when the template is loaded (the spec types it that way, and the Python reference does the same), so brace errors are reported at load time. The generated embedded file name is now built from ASCII characters only, matching the Python reference; some non-ASCII step names previously caused a panic.What is the impact of this change?
Templates with short-form steps get the same diagnostics as full-form steps, at paths that exist in their template. Some templates that previously passed
checkwill now fail it (undefined references, out-of-range literaltimeout/notifyPeriodInSeconds, emptyscriptorargs, aletwithout the EXPR extension). Each of these was already invalid at job creation or run time.How was this change tested?
cargo test --workspace: all pass (twoopenjd-sessionsWindows cross-user tests fail on unmodifiedmainin this environment and are unrelated).crates/openjd-model/tests/integration/test_simple_action_validation.rsassert the full path and message at both template validation and job creation, and assert that no diagnostic for a short-form step contains a desugared path.step.rscover the path-mapping table and step-name sanitization.cargo clippy -D warnings,cargo fmt --check,cargo doc -D warnings, and the copyright header check are clean.Was this change documented?
Yes. Doc comments on all new public items. Specs updated:
specs/model/validation.md(new "Step Scripts and SimpleActions" and "Reporting on desugared forms" sections with the path table),specs/model/job-creation.md,specs/model/template-types.md, andspecs/model/public-api.md.Is this a breaking change?
Yes, for users of the
openjd-modelcrate API:template::SimpleAction.scriptis nowFormatStringinstead ofString. Read the text with.raw().StepTemplate::resolve_syntax_sugar()now returnsOption<StepScript>instead ofResult<Option<StepScript>, ModelError>. It can no longer fail.template::SimpleActionKind,StepTemplate::simple_actions(),StepTemplate::simple_action().No change to the template format, the CLI interface, or the
job::*output types.Does this change impact security?
No. Validation only; no files or processes are created or modified.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.