From 2147dc9169094e1694c3f2b88500fe06b252b0c9 Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Wed, 30 Sep 2026 16:03:03 -0700 Subject: [PATCH] feat(model)!: validate SimpleAction steps on the desugared form with authored error paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` -> 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 `` 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 `` `@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.}}` 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` instead of `Result, ModelError>`. New public `template::SimpleActionKind` and `StepTemplate::simple_actions` / `simple_action`. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- crates/openjd-model/src/error.rs | 19 + .../src/job/create_job/instantiate.rs | 50 +- crates/openjd-model/src/template/mod.rs | 2 +- crates/openjd-model/src/template/step.rs | 463 ++++++++++--- .../validate_v2023_09/format_strings.rs | 497 +++++++------- .../template/validate_v2023_09/structure.rs | 143 ++-- crates/openjd-model/tests/integration.rs | 2 + .../test_simple_action_validation.rs | 647 ++++++++++++++++++ .../integration/test_template_public_api.rs | 2 +- specs/model/job-creation.md | 12 +- specs/model/public-api.md | 37 +- specs/model/template-types.md | 44 +- specs/model/validation.md | 79 ++- 13 files changed, 1604 insertions(+), 393 deletions(-) create mode 100644 crates/openjd-model/tests/integration/test_simple_action_validation.rs diff --git a/crates/openjd-model/src/error.rs b/crates/openjd-model/src/error.rs index 384b84cf..946e73ca 100644 --- a/crates/openjd-model/src/error.rs +++ b/crates/openjd-model/src/error.rs @@ -180,6 +180,25 @@ impl ValidationErrors { self.errors.len() } + /// Append every error in `other`, rewriting its path through `remap`. + /// + /// Used when validation runs against a synthesized structure (a + /// desugared SimpleAction) whose node paths do not exist in the authored + /// template: the checks run into a scratch `ValidationErrors` rooted at + /// the synthesized node, and `remap` translates each relative path onto + /// the field the author actually wrote before the errors join the real + /// collection. Messages and structured detail are carried unchanged. + pub(crate) fn extend_remapped( + &mut self, + other: ValidationErrors, + remap: impl Fn(&[PathElement]) -> Vec, + ) { + for mut err in other.errors { + err.path = remap(&err.path); + self.errors.push(err); + } + } + pub fn into_result(self, model_name: &str) -> Result<(), ModelError> { if self.errors.is_empty() { Ok(()) diff --git a/crates/openjd-model/src/job/create_job/instantiate.rs b/crates/openjd-model/src/job/create_job/instantiate.rs index 8fdc246f..84903335 100644 --- a/crates/openjd-model/src/job/create_job/instantiate.rs +++ b/crates/openjd-model/src/job/create_job/instantiate.rs @@ -76,8 +76,15 @@ pub(super) fn instantiate_step( } } - let script_template = st.resolve_syntax_sugar()?.or_else(|| st.script.clone()); + let script_template = st.resolve_syntax_sugar(); let script = script_template.as_ref().map(convert_step_script); + // When the script came from a SimpleAction (§8), diagnostics on the + // desugared form must point at the field the author wrote. + let sugar_kind = if st.script.is_none() { + st.simple_actions().next().map(|(kind, _)| kind) + } else { + None + }; // Check symbol table for the step script's carried-forward // (session/task-scope) format strings: `step_symtab`'s concrete @@ -112,21 +119,42 @@ pub(super) fn instantiate_step( // `data` — against the check symbol table, where job parameters are // bound to real values. A violation template validation could only // lower-bound is decidable here: fail at submission, not on every - // worker. + // worker. These are exactly the checks pass 8 applies — to the same + // desugared form for a SimpleAction step, with the same path remap + // back onto the authored field. if let (Some(s), Some(cst)) = (&script_template, &check_symtab) { let mut check_errors = ValidationErrors::default(); - let script_path = [ + let step_path = [ PathElement::Field("steps".to_string()), PathElement::Index(step_index), - PathElement::Field("script".to_string()), ]; - crate::template::validate_v2023_09::format_strings::check_carried_forward_step_script( - s, - cst, - ctx, - &script_path, - &mut check_errors, - ); + match sugar_kind { + Some(kind) => { + let mut scratch = ValidationErrors::default(); + crate::template::validate_v2023_09::format_strings::check_carried_forward_step_script( + s, + cst, + ctx, + &[], + &mut scratch, + ); + let sa_path = path_field(&step_path, kind.field_name()); + check_errors.extend_remapped(scratch, |rel| { + let mut p = sa_path.clone(); + p.extend(kind.remap_desugared_path(rel)); + p + }); + } + None => { + crate::template::validate_v2023_09::format_strings::check_carried_forward_step_script( + s, + cst, + ctx, + &path_field(&step_path, "script"), + &mut check_errors, + ); + } + } check_errors.into_result("JobTemplate")?; } diff --git a/crates/openjd-model/src/template/mod.rs b/crates/openjd-model/src/template/mod.rs index 025edbf6..cdd49c13 100644 --- a/crates/openjd-model/src/template/mod.rs +++ b/crates/openjd-model/src/template/mod.rs @@ -44,7 +44,7 @@ pub use expr_parameters::{ ListStringItemConstraints, }; // step -pub use step::{SimpleAction, StepDependency, StepScript, StepTemplate}; +pub use step::{SimpleAction, SimpleActionKind, StepDependency, StepScript, StepTemplate}; // environment pub use environment::{EmbeddedFile, Environment, EnvironmentScript}; // actions diff --git a/crates/openjd-model/src/template/step.rs b/crates/openjd-model/src/template/step.rs index 33649072..3bd11e53 100644 --- a/crates/openjd-model/src/template/step.rs +++ b/crates/openjd-model/src/template/step.rs @@ -9,10 +9,11 @@ use super::constrained_strings::Description; use super::environment::{EmbeddedFile, Environment}; use super::host_requirements::HostRequirements; use super::task_parameters::StepParameterSpaceDefinition; +use crate::error::PathElement; use crate::format_string::FormatString; use serde::Deserialize; -/// SimpleAction syntax sugar (FEATURE_BUNDLE_1). +/// SimpleAction syntax sugar (FEATURE_BUNDLE_1, Template Schemas §8). /// Allows specifying a script interpreter directly instead of a full StepScript. #[derive(Debug, Clone, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] @@ -20,8 +21,10 @@ pub struct SimpleAction { /// Let bindings evaluated once per task (requires EXPR extension). #[serde(rename = "let")] pub let_bindings: Option>, - /// The script content to execute. Required. - pub script: String, + /// The script content to execute. Required. A `` + /// (`@fmtstring[host]`), so malformed `{{ ... }}` syntax is rejected at + /// parse time exactly like every other format-string field. + pub script: FormatString, /// Additional arguments to pass to the interpreter. pub args: Option>, /// Maximum allowed runtime in seconds. @@ -30,6 +33,200 @@ pub struct SimpleAction { pub cancelation: Option, } +/// The interpreter key a [`SimpleAction`] was written under (§8). +/// +/// Each kind fixes the desugared `command`, the embedded file's extension, +/// and the interpreter arguments that precede the generated file reference +/// in `args`. It also knows how to map a validation-error path on the +/// desugared [`StepScript`] back onto the field the author actually wrote, +/// so diagnostics never name a node that does not exist in the template. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum SimpleActionKind { + /// `python:` — `command: python`, `.py`. + Python, + /// `bash:` — `command: bash`, `.sh`. + Bash, + /// `cmd:` — `command: cmd`, `.bat`, args prefixed with `/C`. + Cmd, + /// `powershell:` — `command: powershell`, `.ps1`, args prefixed with `-File`. + Powershell, + /// `node:` — `command: node`, `.js`. + Node, +} + +impl SimpleActionKind { + /// Every kind, in the order [`StepTemplate::resolve_syntax_sugar`] + /// considers them. + pub const ALL: [SimpleActionKind; 5] = [ + SimpleActionKind::Python, + SimpleActionKind::Bash, + SimpleActionKind::Cmd, + SimpleActionKind::Powershell, + SimpleActionKind::Node, + ]; + + /// The step field name this kind is written under — also the desugared + /// `command` (§8 requires the lowercase interpreter name on `PATH`). + #[must_use] + pub fn field_name(self) -> &'static str { + match self { + SimpleActionKind::Python => "python", + SimpleActionKind::Bash => "bash", + SimpleActionKind::Cmd => "cmd", + SimpleActionKind::Powershell => "powershell", + SimpleActionKind::Node => "node", + } + } + + /// File extension of the generated embedded file. + #[must_use] + pub fn file_extension(self) -> &'static str { + match self { + SimpleActionKind::Python => ".py", + SimpleActionKind::Bash => ".sh", + SimpleActionKind::Cmd => ".bat", + SimpleActionKind::Powershell => ".ps1", + SimpleActionKind::Node => ".js", + } + } + + /// Interpreter arguments inserted before the generated file reference. + #[must_use] + pub fn arg_prefix(self) -> &'static [&'static str] { + match self { + SimpleActionKind::Cmd => &["/C"], + SimpleActionKind::Powershell => &["-File"], + _ => &[], + } + } + + /// Number of synthesized leading `args` entries in the desugared action: + /// the interpreter prefix plus the `{{Task.File.}}` reference. + /// The author's own `args[k]` lands at desugared `args[k + offset]`. + #[must_use] + pub fn synthetic_arg_count(self) -> usize { + self.arg_prefix().len() + 1 + } + + /// Expand `sa` into the equivalent [`StepScript`] per §8. `step_name` is + /// sanitized into the generated embedded file's name: every character + /// outside `[A-Za-z0-9]` becomes `_` (as in `openjd-model-for-python`), + /// truncated to 200 characters, `_`-prefixed if it would start with a + /// digit, suffixed `_script`. The result is always a valid identifier, + /// so the `{{Task.File.}}` reference always parses. + #[must_use] + pub fn desugar(self, step_name: &str, sa: &SimpleAction) -> StepScript { + let safe_name: String = step_name + .chars() + .map(|c| if c.is_ascii_alphanumeric() { c } else { '_' }) + .take(200) + .collect(); + let safe_name = if safe_name.starts_with(|c: char| c.is_ascii_digit()) { + format!("_{safe_name}") + } else { + safe_name + }; + let embedded_name = format!("{safe_name}_script"); + let filename = format!("{embedded_name}{}", self.file_extension()); + let file_ref = format!("{{{{Task.File.{embedded_name}}}}}"); + + let mut args = Vec::new(); + for prefix_arg in self.arg_prefix() { + args.push(FormatString::new(prefix_arg).expect("literal arg prefix")); + } + args.push(FormatString::new(&file_ref).expect("generated Task.File reference")); + if let Some(user_args) = &sa.args { + args.extend(user_args.iter().cloned()); + } + + StepScript { + let_bindings: sa.let_bindings.clone(), + actions: StepActions { + on_run: Action { + command: FormatString::new(self.field_name()).expect("literal command"), + args: Some(args), + cancelation: sa.cancelation.clone(), + timeout: sa.timeout.clone(), + }, + }, + embedded_files: Some(vec![EmbeddedFile { + name: embedded_name, + file_type: crate::types::FileType::Text, + filename: Some(filename), + data: Some(sa.script.clone()), + runnable: Some(true), + end_of_line: None, + }]), + } + } + + /// Map a validation-error path *relative to the desugared `script` node* + /// back onto the corresponding path relative to the `SimpleAction` the + /// author wrote. Validation runs on the desugared form (so the sugar and + /// its expansion are checked by exactly the same code), but every + /// diagnostic must point at a node that exists in the template: + /// + /// | desugared (relative to `script`) | sugar (relative to ``) | + /// |--------------------------------------------|------------------------------| + /// | `let[...]` | `let[...]` | + /// | `actions -> onRun -> args` | `args` | + /// | `actions -> onRun -> args[k]`, `k ≥ offset`| `args[k - offset]` | + /// | `actions -> onRun -> timeout` | `timeout` | + /// | `actions -> onRun -> cancelation ...` | `cancelation ...` | + /// | `embeddedFiles[0] -> data` | `script` | + /// | anything synthesized (`command`, the generated file's `name` / `filename`, the `Task.File` arg, `actions -> onRun` itself) | the `` field itself | + /// + /// where `offset` is [`Self::synthetic_arg_count`]. + #[must_use] + pub fn remap_desugared_path(self, rel: &[PathElement]) -> Vec { + use PathElement::{Field, Index}; + match rel { + [Field(f), rest @ ..] if f == "let" => { + let mut out = vec![Field("let".into())]; + out.extend_from_slice(rest); + out + } + [Field(actions), Field(on_run), rest @ ..] + if actions == "actions" && on_run == "onRun" => + { + match rest { + [Field(f)] if f == "args" => vec![Field("args".into())], + [Field(f), Index(k), tail @ ..] if f == "args" => { + let offset = self.synthetic_arg_count(); + if *k < offset { + // A synthesized interpreter argument — nothing + // the author wrote corresponds to it. + Vec::new() + } else { + let mut out = vec![Field("args".into()), Index(k - offset)]; + out.extend_from_slice(tail); + out + } + } + [Field(f), tail @ ..] if f == "timeout" || f == "cancelation" => { + let mut out = vec![Field(f.clone())]; + out.extend_from_slice(tail); + out + } + // `command` and the action node itself are synthesized. + _ => Vec::new(), + } + } + [Field(files), Index(0), Field(data), tail @ ..] + if files == "embeddedFiles" && data == "data" => + { + let mut out = vec![Field("script".into())]; + out.extend_from_slice(tail); + out + } + // The generated file's `name`/`filename`/`type`, the + // `embeddedFiles` list itself, and anything unforeseen: point + // at the sugar field as a whole rather than at a phantom node. + _ => Vec::new(), + } + } +} + /// §3 StepTemplate #[derive(Debug, Clone, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] @@ -52,77 +249,40 @@ pub struct StepTemplate { } impl StepTemplate { - /// De-sugar SimpleAction syntax into equivalent StepScript. - /// If the step already has a `script` field, returns `Ok(Some(clone))`. - /// If it uses a SimpleAction (bash/python/cmd/powershell/node), transforms - /// it into a StepScript with an embedded file and onRun action. - /// Returns `Err` if the SimpleAction script contains malformed format string syntax. - pub fn resolve_syntax_sugar(&self) -> Result, crate::ModelError> { - if let Some(script) = &self.script { - return Ok(Some(script.clone())); - } - - let interpreters: &[(&str, &str, &[&str], Option<&SimpleAction>)] = &[ - ("python", ".py", &[], self.python.as_ref()), - ("bash", ".sh", &[], self.bash.as_ref()), - ("cmd", ".bat", &["/C"], self.cmd.as_ref()), - ("powershell", ".ps1", &["-File"], self.powershell.as_ref()), - ("node", ".js", &[], self.node.as_ref()), - ]; - - for &(command, ext, arg_prefix, sa_opt) in interpreters { - let Some(sa) = sa_opt else { continue }; - - let safe_name: String = self - .name - .chars() - .map(|c| if c.is_alphanumeric() { c } else { '_' }) - .take(200) - .collect(); - let safe_name = if safe_name.starts_with(|c: char| c.is_ascii_digit()) { - format!("_{safe_name}") - } else { - safe_name - }; - let embedded_name = format!("{safe_name}_script"); - let filename = format!("{embedded_name}{ext}"); - let file_ref = format!("{{{{Task.File.{embedded_name}}}}}"); - - let mut args = Vec::new(); - for prefix_arg in arg_prefix { - args.push(FormatString::new(prefix_arg).unwrap()); - } - args.push(FormatString::new(&file_ref).unwrap()); - if let Some(user_args) = &sa.args { - args.extend(user_args.iter().cloned()); - } + /// The SimpleAction fields this step sets, paired with their kind, in + /// [`SimpleActionKind::ALL`] order. A valid step has at most one (pass 7 + /// rejects more), but validation iterates them all so every field the + /// author wrote is checked and reported at its own path. + pub fn simple_actions(&self) -> impl Iterator { + SimpleActionKind::ALL + .into_iter() + .filter_map(move |kind| self.simple_action(kind).map(|sa| (kind, sa))) + } - return Ok(Some(StepScript { - let_bindings: sa.let_bindings.clone(), - actions: StepActions { - on_run: Action { - command: FormatString::new(command).unwrap(), - args: Some(args), - cancelation: sa.cancelation.clone(), - timeout: sa.timeout.clone(), - }, - }, - embedded_files: Some(vec![EmbeddedFile { - name: embedded_name, - file_type: crate::types::FileType::Text, - filename: Some(filename), - data: Some(FormatString::new(&sa.script).map_err(|e| { - crate::ModelError::DecodeValidation(format!( - "SimpleAction script format string error: {e}" - )) - })?), - runnable: Some(true), - end_of_line: None, - }]), - })); + /// The SimpleAction written under `kind`, if any. + #[must_use] + pub fn simple_action(&self, kind: SimpleActionKind) -> Option<&SimpleAction> { + match kind { + SimpleActionKind::Python => self.python.as_ref(), + SimpleActionKind::Bash => self.bash.as_ref(), + SimpleActionKind::Cmd => self.cmd.as_ref(), + SimpleActionKind::Powershell => self.powershell.as_ref(), + SimpleActionKind::Node => self.node.as_ref(), } + } - Ok(None) + /// The step's script as a [`StepScript`]: a clone of `script` when + /// present, otherwise the first SimpleAction field (in + /// [`SimpleActionKind::ALL`] order) desugared per §8. `None` when the + /// step has neither — a structural error pass 6 reports. + #[must_use] + pub fn resolve_syntax_sugar(&self) -> Option { + if let Some(script) = &self.script { + return Some(script.clone()); + } + self.simple_actions() + .next() + .map(|(kind, sa)| kind.desugar(&self.name, sa)) } } @@ -145,42 +305,165 @@ pub struct StepScript { #[cfg(test)] mod tests { - use super::StepTemplate; + use super::{SimpleActionKind, StepTemplate}; + use crate::error::PathElement::{Field, Index}; + + fn f(s: &str) -> crate::error::PathElement { + Field(s.to_string()) + } #[test] - fn resolve_syntax_sugar_returns_error_for_malformed_format_string() { - let step: StepTemplate = serde_saphyr::from_str( + fn malformed_script_format_string_is_rejected_at_parse_time() { + // `script` is a `` (@fmtstring[host]): brace mismatches + // fail deserialization like every other format-string field, rather + // than surviving to job creation. + let result: Result = serde_saphyr::from_str( r#" name: TestStep bash: script: "echo '{{broken'" "#, - ) - .unwrap(); - - let result = step.resolve_syntax_sugar(); - assert!( - result.is_err(), - "resolve_syntax_sugar should return Err for malformed format string" ); + let err = result + .expect_err("malformed script must not parse") + .to_string(); + assert!(err.contains("Braces mismatch"), "got: {err}"); } #[test] - fn resolve_syntax_sugar_ok_for_valid_script() { + fn resolve_syntax_sugar_desugars_first_simple_action() { let step: StepTemplate = serde_saphyr::from_str( r#" - name: TestStep - bash: - script: "echo hello" + name: Test Step + powershell: + script: "Write-Host hi" + args: ["--flag"] "#, ) .unwrap(); - let result = step.resolve_syntax_sugar(); - assert!(result.is_ok(), "valid script should succeed"); - assert!( - result.unwrap().is_some(), - "bash step should produce a StepScript" + let script = step + .resolve_syntax_sugar() + .expect("powershell step desugars"); + assert_eq!(script.actions.on_run.command.raw(), "powershell"); + let args: Vec<&str> = script + .actions + .on_run + .args + .as_ref() + .unwrap() + .iter() + .map(|a| a.raw()) + .collect(); + assert_eq!(args, ["-File", "{{Task.File.Test_Step_script}}", "--flag"]); + let file = &script.embedded_files.as_ref().unwrap()[0]; + assert_eq!(file.name, "Test_Step_script"); + assert_eq!(file.filename.as_deref(), Some("Test_Step_script.ps1")); + assert_eq!(file.data.as_ref().unwrap().raw(), "Write-Host hi"); + } + + #[test] + fn resolve_syntax_sugar_none_without_script_or_sugar() { + let step: StepTemplate = serde_saphyr::from_str("name: S\n").unwrap(); + assert!(step.resolve_syntax_sugar().is_none()); + } + + #[test] + fn desugar_sanitizes_step_name_to_ascii_identifier() { + // `²` and `½` are `char::is_alphanumeric` but not identifier + // characters; `١` (Arabic-Indic one) is a non-ASCII digit. Each + // used to panic at the generated `{{Task.File.}}` reference. + for (name, expected) in [ + ("²x", "_x_script"), + ("a½", "a__script"), + ("١", "__script"), + ("9lives", "_9lives_script"), + ("Test Step", "Test_Step_script"), + ("ünïcödé", "_n_c_d__script"), + ] { + let step: StepTemplate = + serde_saphyr::from_str(&format!("name: \"{name}\"\nbash:\n script: echo\n")) + .unwrap(); + let script = step.resolve_syntax_sugar().unwrap(); + let file = &script.embedded_files.as_ref().unwrap()[0]; + assert_eq!(file.name, expected, "step name {name:?}"); + assert_eq!( + script.actions.on_run.args.as_ref().unwrap()[0].raw(), + format!("{{{{Task.File.{expected}}}}}") + ); + } + } + + #[test] + fn remap_authored_fields() { + let k = SimpleActionKind::Bash; + assert_eq!(k.remap_desugared_path(&[]), vec![]); + assert_eq!( + k.remap_desugared_path(&[f("let"), Index(2)]), + vec![f("let"), Index(2)] + ); + assert_eq!( + k.remap_desugared_path(&[f("actions"), f("onRun"), f("args")]), + vec![f("args")] + ); + assert_eq!( + k.remap_desugared_path(&[f("actions"), f("onRun"), f("timeout")]), + vec![f("timeout")] + ); + assert_eq!( + k.remap_desugared_path(&[f("actions"), f("onRun"), f("cancelation")]), + vec![f("cancelation")] + ); + assert_eq!( + k.remap_desugared_path(&[f("embeddedFiles"), Index(0), f("data")]), + vec![f("script")] + ); + } + + #[test] + fn remap_args_offsets_by_synthetic_arg_count() { + // bash: [, user0, user1] + let bash = SimpleActionKind::Bash; + assert_eq!(bash.synthetic_arg_count(), 1); + assert_eq!( + bash.remap_desugared_path(&[f("actions"), f("onRun"), f("args"), Index(1)]), + vec![f("args"), Index(0)] + ); + // The synthesized Task.File reference has no authored counterpart. + assert_eq!( + bash.remap_desugared_path(&[f("actions"), f("onRun"), f("args"), Index(0)]), + vec![] + ); + // cmd: ["/C", , user0] + let cmd = SimpleActionKind::Cmd; + assert_eq!(cmd.synthetic_arg_count(), 2); + assert_eq!( + cmd.remap_desugared_path(&[f("actions"), f("onRun"), f("args"), Index(2)]), + vec![f("args"), Index(0)] + ); + assert_eq!( + cmd.remap_desugared_path(&[f("actions"), f("onRun"), f("args"), Index(1)]), + vec![] + ); + assert_eq!(SimpleActionKind::Powershell.synthetic_arg_count(), 2); + } + + #[test] + fn remap_synthesized_nodes_collapse_to_sugar_field() { + let k = SimpleActionKind::Node; + assert_eq!( + k.remap_desugared_path(&[f("actions"), f("onRun"), f("command")]), + vec![] + ); + assert_eq!(k.remap_desugared_path(&[f("actions"), f("onRun")]), vec![]); + assert_eq!(k.remap_desugared_path(&[f("embeddedFiles")]), vec![]); + assert_eq!( + k.remap_desugared_path(&[f("embeddedFiles"), Index(0), f("name")]), + vec![] + ); + assert_eq!( + k.remap_desugared_path(&[f("embeddedFiles"), Index(0), f("filename")]), + vec![] ); } } diff --git a/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs b/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs index 074f06ca..c7677492 100644 --- a/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs +++ b/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs @@ -238,13 +238,7 @@ fn build_task_scope_symtab( // so the runtime allocates embedded file paths — defining Task.File.* — // before evaluating `let`, mirroring the environment runner's Env.File.* // ordering. - if let Some(script) = step - .resolve_syntax_sugar() - .ok() - .flatten() - .as_ref() - .or(step.script.as_ref()) - { + if let Some(script) = step.resolve_syntax_sugar() { if let Some(files) = &script.embedded_files { for f in files { symtab @@ -1092,6 +1086,218 @@ fn validate_fs_with( } } +/// Pass-8 checks for one step script (§3.5), rooted at `script_path`: +/// script-level `let` bindings (evaluated into `task_symtab`), the +/// EXPR-gated complex-expression rejection, the `onRun` action's +/// `command`/`args` (task scope), `timeout`/`cancelation` (template scope), +/// each embedded file's `data` (task scope), and comprehension-variable +/// rules against the step + script `let` names. +/// +/// Shared by the authored `script` field and the desugared SimpleAction +/// forms (§8); the latter run it into a scratch collection rooted at `[]` +/// and re-root the paths onto the sugar field via +/// [`SimpleActionKind::remap_desugared_path`]. +#[allow(clippy::too_many_arguments)] +fn validate_step_script_format_strings( + script: &StepScript, + script_path: &[PathElement], + step: &StepTemplate, + task_symtab: &mut SymbolTable, + step_template_symtab: &SymbolTable, + host_ev: &FsEval<'_>, + host_profile: &openjd_expr::ExprProfile, + template_ev: &FsEval<'_>, + ctx: &ValidationContext, + expr_active: bool, + errors: &mut ValidationErrors, +) { + // Script-level let bindings (TASK scope — host_lib). Task.File.* + // is in scope: file paths are allocated before `let` evaluation + // at runtime (filenames are plain strings, so allocation cannot + // depend on `let` values). + if let Some(bindings) = &script.let_bindings { + let let_path = path_field(script_path, "let"); + if !expr_active { + errors.add(&let_path, "'let' requires the EXPR extension."); + } else { + let enclosing: HashSet = step + .let_bindings + .as_ref() + .map(|bs| { + bs.iter() + .filter_map(|b| b.find('=').map(|eq| b[..eq].trim().to_string())) + .collect() + }) + .unwrap_or_default(); + let mut script_let_names = HashSet::new(); + validate_let_bindings( + bindings, + &let_path, + &enclosing, + &mut script_let_names, + task_symtab, + host_ev, + host_profile, + errors, + ); + } + } + + // Complex expressions: reject if not EXPR + if !expr_active { + let action_path = path_field(&path_field(script_path, "actions"), "onRun"); + if script.actions.on_run.command.has_complex_expressions() { + errors.add( + &path_field(&action_path, "command"), + "complex expressions require the EXPR extension.", + ); + } + if let Some(args) = &script.actions.on_run.args { + let args_path = path_field(&action_path, "args"); + for (j, arg) in args.iter().enumerate() { + if arg.has_complex_expressions() { + errors.add( + &path_index(&args_path, j), + "complex expressions require the EXPR extension.", + ); + } + } + } + } + + // Validate all format string references (both base and EXPR) + let action_path = path_field(&path_field(script_path, "actions"), "onRun"); + validate_action_fs( + &script.actions.on_run, + task_symtab, + host_ev, + &action_path, + ctx.caller_limits.max_resolved_arg_len, + errors, + ); + + // Timeout and notifyPeriodInSeconds are plain @fmtstring + // (resolved at job creation, before any session exists), so + // they validate against the template-scope symtab: no + // Session.*, no Task.*, no Env.File.*, no host functions. + if let Some(timeout) = &script.actions.on_run.timeout { + validate_fs_with( + timeout, + step_template_symtab, + template_ev, + &path_field(&action_path, "timeout"), + Some(&TIMEOUT_CONSTRAINT), + errors, + ); + } + let (mode_fs, notify_fs) = match &script.actions.on_run.cancelation { + Some(CancelationMode::NotifyThenTerminate { + notify_period_in_seconds, + }) => (None, notify_period_in_seconds.as_ref()), + Some(CancelationMode::DeferredMode { + mode, + notify_period_in_seconds, + }) => (Some(mode), notify_period_in_seconds.as_ref()), + _ => (None, None), + }; + if let Some(mode) = mode_fs { + validate_fs_with( + mode, + step_template_symtab, + template_ev, + &path_field(&action_path, "cancelation"), + Some(&ResolvedConstraint::CancelationMode), + errors, + ); + } + if let Some(notify) = notify_fs { + validate_fs_with( + notify, + step_template_symtab, + template_ev, + &path_field(&action_path, "cancelation"), + Some(&NOTIFY_PERIOD_CONSTRAINT), + errors, + ); + } + + // Embedded files + if let Some(files) = &script.embedded_files { + let files_path = path_field(script_path, "embeddedFiles"); + let data_constraint = ctx + .caller_limits + .max_resolved_data_len + .map(|max_len| ResolvedConstraint::ResolvedString { max_len }); + for (j, f) in files.iter().enumerate() { + let f_path = path_index(&files_path, j); + if let Some(data) = &f.data { + validate_fs_with( + data, + task_symtab, + host_ev, + &path_field(&f_path, "data"), + data_constraint.as_ref(), + errors, + ); + } + // `filename` is a plain string per the 2023-09 schema + // (not @fmtstring) — no format-string validation. + } + } + + // EXPR-only: comprehension variable validation + if expr_active { + let mut all_let_names: HashSet = HashSet::new(); + if let Some(bindings) = &step.let_bindings { + for b in bindings { + if let Some(eq) = b.find('=') { + all_let_names.insert(b[..eq].trim().to_string()); + } + } + } + if let Some(bindings) = &script.let_bindings { + for b in bindings { + if let Some(eq) = b.find('=') { + all_let_names.insert(b[..eq].trim().to_string()); + } + } + } + if !all_let_names.is_empty() { + if let Err(e) = script + .actions + .on_run + .command + .validate_comprehension_vars(&all_let_names) + { + errors.add(&path_field(&action_path, "command"), e.to_string()); + } + if let Some(args) = &script.actions.on_run.args { + let args_path = path_field(&action_path, "args"); + for (j, arg) in args.iter().enumerate() { + if let Err(e) = arg.validate_comprehension_vars(&all_let_names) { + errors.add(&path_index(&args_path, j), e.to_string()); + } + } + } + // Embedded-file `data` sees the same `let` names as `args` + // (a SimpleAction's `script` desugars to it). + if let Some(files) = &script.embedded_files { + let files_path = path_field(script_path, "embeddedFiles"); + for (j, f) in files.iter().enumerate() { + if let Some(data) = &f.data { + if let Err(e) = data.validate_comprehension_vars(&all_let_names) { + errors.add( + &path_field(&path_index(&files_path, j), "data"), + e.to_string(), + ); + } + } + } + } + } + } +} + /// Validate a format string in an action (command + args). When the /// caller opted into `CallerLimits::max_resolved_arg_len`, the resolved /// command string and each argv entry the args produce are bounded by it @@ -1322,6 +1528,7 @@ pub fn validate_format_strings( errors: &mut ValidationErrors, ) { let expr_active = ctx.profile.has_extension(ModelExtension::Expr); + let fb1_active = ctx.profile.has_extension(ModelExtension::FeatureBundle1); // Template/task-range validation uses HostContext::None (host functions // are not available in those scopes). Session/task scopes use // HostContext::Unresolved so apply_path_mapping type-checks. @@ -1799,177 +2006,56 @@ pub fn validate_format_strings( } if let Some(script) = &step.script { - let script_path = path_field(&step_path, "script"); - - // Script-level let bindings (TASK scope — host_lib). Task.File.* - // is in scope: file paths are allocated before `let` evaluation - // at runtime (filenames are plain strings, so allocation cannot - // depend on `let` values). - if let Some(bindings) = &script.let_bindings { - let let_path = path_field(&script_path, "let"); - if !expr_active { - errors.add(&let_path, "'let' requires the EXPR extension."); - } else { - let enclosing: HashSet = step - .let_bindings - .as_ref() - .map(|bs| { - bs.iter() - .filter_map(|b| b.find('=').map(|eq| b[..eq].trim().to_string())) - .collect() - }) - .unwrap_or_default(); - let mut script_let_names = HashSet::new(); - validate_let_bindings( - bindings, - &let_path, - &enclosing, - &mut script_let_names, - &mut task_symtab, - &host_ev, - &host_profile, - errors, - ); - } - } - - // Complex expressions: reject if not EXPR - if !expr_active { - let action_path = path_field(&path_field(&script_path, "actions"), "onRun"); - if script.actions.on_run.command.has_complex_expressions() { - errors.add( - &path_field(&action_path, "command"), - "complex expressions require the EXPR extension.", - ); - } - if let Some(args) = &script.actions.on_run.args { - let args_path = path_field(&action_path, "args"); - for (j, arg) in args.iter().enumerate() { - if arg.has_complex_expressions() { - errors.add( - &path_index(&args_path, j), - "complex expressions require the EXPR extension.", - ); - } - } - } - } - - // Validate all format string references (both base and EXPR) - let action_path = path_field(&path_field(&script_path, "actions"), "onRun"); - validate_action_fs( - &script.actions.on_run, - &task_symtab, + validate_step_script_format_strings( + script, + &path_field(&step_path, "script"), + step, + &mut task_symtab, + &step_template_symtab, &host_ev, - &action_path, - ctx.caller_limits.max_resolved_arg_len, + &host_profile, + &template_ev, + ctx, + expr_active, errors, ); + } - // Timeout and notifyPeriodInSeconds are plain @fmtstring - // (resolved at job creation, before any session exists), so - // they validate against the template-scope symtab: no - // Session.*, no Task.*, no Env.File.*, no host functions. - if let Some(timeout) = &script.actions.on_run.timeout { - validate_fs_with( - timeout, - &step_template_symtab, - &template_ev, - &path_field(&action_path, "timeout"), - Some(&TIMEOUT_CONSTRAINT), - errors, - ); - } - let (mode_fs, notify_fs) = match &script.actions.on_run.cancelation { - Some(CancelationMode::NotifyThenTerminate { - notify_period_in_seconds, - }) => (None, notify_period_in_seconds.as_ref()), - Some(CancelationMode::DeferredMode { - mode, - notify_period_in_seconds, - }) => (Some(mode), notify_period_in_seconds.as_ref()), - _ => (None, None), - }; - if let Some(mode) = mode_fs { - validate_fs_with( - mode, - &step_template_symtab, - &template_ev, - &path_field(&action_path, "cancelation"), - Some(&ResolvedConstraint::CancelationMode), - errors, - ); - } - if let Some(notify) = notify_fs { - validate_fs_with( - notify, + // SimpleAction fields (§8, FEATURE_BUNDLE_1): validate the desugared + // script with exactly the same code, into a scratch collection + // rooted at the synthesized `script` node, then re-root every error + // onto the field the author wrote (`steps[i] -> bash -> script`, + // `-> args[k]`, `-> timeout`, ...). Each field gets its own copy of + // the task symtab so its `let` bindings do not leak into a sibling. + // Skipped when the extension is off (pass 7 rejects the field) and + // when the step also has `script` (pass 7 rejects the combination, + // and the task symtab's `Task.File.*` were seeded from the authored + // script — validating the sugar against it would report the + // generated file reference as undefined, naming a node the author + // never wrote). + if fb1_active && step.script.is_none() { + for (kind, sa) in step.simple_actions() { + let sa_path = path_field(&step_path, kind.field_name()); + let mut sa_symtab = task_symtab.clone(); + let mut scratch = ValidationErrors::default(); + validate_step_script_format_strings( + &kind.desugar(&step.name, sa), + &[], + step, + &mut sa_symtab, &step_template_symtab, + &host_ev, + &host_profile, &template_ev, - &path_field(&action_path, "cancelation"), - Some(&NOTIFY_PERIOD_CONSTRAINT), - errors, + ctx, + expr_active, + &mut scratch, ); - } - - // Embedded files - if let Some(files) = &script.embedded_files { - let files_path = path_field(&script_path, "embeddedFiles"); - let data_constraint = ctx - .caller_limits - .max_resolved_data_len - .map(|max_len| ResolvedConstraint::ResolvedString { max_len }); - for (j, f) in files.iter().enumerate() { - let f_path = path_index(&files_path, j); - if let Some(data) = &f.data { - validate_fs_with( - data, - &task_symtab, - &host_ev, - &path_field(&f_path, "data"), - data_constraint.as_ref(), - errors, - ); - } - // `filename` is a plain string per the 2023-09 schema - // (not @fmtstring) — no format-string validation. - } - } - - // EXPR-only: comprehension variable validation - if expr_active { - let mut all_let_names: HashSet = HashSet::new(); - if let Some(bindings) = &step.let_bindings { - for b in bindings { - if let Some(eq) = b.find('=') { - all_let_names.insert(b[..eq].trim().to_string()); - } - } - } - if let Some(bindings) = &script.let_bindings { - for b in bindings { - if let Some(eq) = b.find('=') { - all_let_names.insert(b[..eq].trim().to_string()); - } - } - } - if !all_let_names.is_empty() { - if let Err(e) = script - .actions - .on_run - .command - .validate_comprehension_vars(&all_let_names) - { - errors.add(&path_field(&action_path, "command"), e.to_string()); - } - if let Some(args) = &script.actions.on_run.args { - let args_path = path_field(&action_path, "args"); - for (j, arg) in args.iter().enumerate() { - if let Err(e) = arg.validate_comprehension_vars(&all_let_names) { - errors.add(&path_index(&args_path, j), e.to_string()); - } - } - } - } + errors.extend_remapped(scratch, |rel| { + let mut p = sa_path.clone(); + p.extend(kind.remap_desugared_path(rel)); + p + }); } } @@ -2054,75 +2140,6 @@ pub fn validate_format_strings( ); } } - - // SimpleAction let bindings (requires both FB1 and EXPR) - if expr_active { - for sa in [ - &step.bash, - &step.python, - &step.cmd, - &step.powershell, - &step.node, - ] - .into_iter() - .flatten() - { - let mut sa_let_names: HashSet = HashSet::new(); - if let Some(bindings) = &step.let_bindings { - for b in bindings { - if let Some(eq) = b.find('=') { - sa_let_names.insert(b[..eq].trim().to_string()); - } - } - } - if let Some(let_bindings) = &sa.let_bindings { - if ctx.profile.has_extension(ModelExtension::FeatureBundle1) { - let enclosing: HashSet = step - .let_bindings - .as_ref() - .map(|bs| { - bs.iter() - .filter_map(|b| { - b.find('=').map(|eq| b[..eq].trim().to_string()) - }) - .collect() - }) - .unwrap_or_default(); - let mut new_names = HashSet::new(); - validate_let_bindings( - let_bindings, - &step_path, - &enclosing, - &mut new_names, - &mut task_symtab, - &host_ev, - &host_profile, - errors, - ); - sa_let_names.extend(new_names); - } - } - if !sa_let_names.is_empty() { - match FormatString::new(&sa.script) { - Ok(fs) => { - if let Err(e) = fs.validate_comprehension_vars(&sa_let_names) { - errors.add(&step_path, e.to_string()); - } - } - Err(e) => { - errors.add(&step_path, format!("SimpleAction script: {e}")); - } - } - if let Some(args) = &sa.args { - for arg in args { - if let Err(e) = arg.validate_comprehension_vars(&sa_let_names) { - errors.add(&step_path, e.to_string()); - } - } - } - } - } - } } // Environment script comprehension validation (EXPR only) diff --git a/crates/openjd-model/src/template/validate_v2023_09/structure.rs b/crates/openjd-model/src/template/validate_v2023_09/structure.rs index a39a6616..8677ea9a 100644 --- a/crates/openjd-model/src/template/validate_v2023_09/structure.rs +++ b/crates/openjd-model/src/template/validate_v2023_09/structure.rs @@ -162,6 +162,9 @@ pub fn validate_structure( // Step validation let all_step_names: HashSet = jt.steps.iter().map(|s| s.name.clone()).collect(); + let fb1_active = ctx + .profile + .has_extension(crate::types::ModelExtension::FeatureBundle1); let mut step_names = HashSet::new(); for (i, step) in jt.steps.iter().enumerate() { let step_path = vec![PathElement::Field("steps".into()), PathElement::Index(i)]; @@ -293,51 +296,47 @@ pub fn validate_structure( // Script actions if let Some(script) = &step.script { - let script_path = path_field(&step_path, "script"); - let action_path = path_field(&path_field(&script_path, "actions"), "onRun"); - validate_action(&script.actions.on_run, &action_path, limits, rules, errors); - - // Task.File.* references - let file_names: HashSet = script - .embedded_files - .as_ref() - .map(|files| files.iter().map(|f| f.name.clone()).collect()) - .unwrap_or_default(); - let all_fs: Vec<&openjd_expr::FormatString> = { - let mut v = vec![&script.actions.on_run.command]; - if let Some(args) = &script.actions.on_run.args { - v.extend(args.iter()); - } - v - }; - for fs in &all_fs { - for name in fs.expression_names() { - if let Some(tail) = name.strip_prefix("Task.File.") { - // Only the first dotted segment is the embedded-file - // key. `Task.File.` is `path` typed, so any - // further segments are property access on that value — - // RFC 0005 uses `{{Task.File.Run.name}}` directly. - // Embedded file names are identifiers (no dots, see - // the name validation below), so the first segment is - // unambiguous. - let file_name = tail.split_once('.').map_or(tail, |(key, _)| key); - if !file_names.contains(file_name) { - errors.add( - &script_path, - format!("references undefined embedded file '{file_name}'."), - ); - } - } - } - } + validate_step_script( + script, + &path_field(&step_path, "script"), + limits, + rules, + errors, + ); + } - // Embedded files - if let Some(files) = &script.embedded_files { - let files_path = path_field(&script_path, "embeddedFiles"); - if files.is_empty() { - errors.add(&files_path, "must not be empty."); + // SimpleAction fields (§8): validate the desugared script with the + // same code, then re-root every error onto the authored field so no + // diagnostic names a synthesized node. Skipped when the extension + // is off (pass 7 rejects the field outright) and when the step also + // has `script` (pass 7 rejects the combination; validating the sugar + // against a step whose `Task.File.*` come from the authored script + // would only add noise about the generated file). + if fb1_active && step.script.is_none() { + for (kind, sa) in step.simple_actions() { + let sa_path = path_field(&step_path, kind.field_name()); + // The desugared `args` always holds the generated file + // reference, so the shared check cannot see an authored + // empty list (Python's `ArgListType` requires ≥ 1). + if sa.args.as_ref().is_some_and(Vec::is_empty) { + errors.add( + &path_field(&sa_path, "args"), + "if provided, must not be empty.", + ); } - validate_embedded_files(files, &files_path, errors); + let mut scratch = ValidationErrors::default(); + validate_step_script( + &kind.desugar(&step.name, sa), + &[], + limits, + rules, + &mut scratch, + ); + errors.extend_remapped(scratch, |rel| { + let mut p = sa_path.clone(); + p.extend(kind.remap_desugared_path(rel)); + p + }); } } } @@ -451,6 +450,64 @@ pub fn validate_single_environment( } } +/// Structural checks for a step script (§3.5) rooted at `script_path`: the +/// `onRun` action, `Task.File.*` references against the embedded file names, +/// and the embedded files themselves. Shared by the authored `script` field +/// and the desugared SimpleAction forms. +fn validate_step_script( + script: &StepScript, + script_path: &[PathElement], + limits: &super::EffectiveLimits, + rules: &EffectiveRules, + errors: &mut ValidationErrors, +) { + let action_path = path_field(&path_field(script_path, "actions"), "onRun"); + validate_action(&script.actions.on_run, &action_path, limits, rules, errors); + + // Task.File.* references + let file_names: HashSet = script + .embedded_files + .as_ref() + .map(|files| files.iter().map(|f| f.name.clone()).collect()) + .unwrap_or_default(); + let all_fs: Vec<&openjd_expr::FormatString> = { + let mut v = vec![&script.actions.on_run.command]; + if let Some(args) = &script.actions.on_run.args { + v.extend(args.iter()); + } + v + }; + for fs in &all_fs { + for name in fs.expression_names() { + if let Some(tail) = name.strip_prefix("Task.File.") { + // Only the first dotted segment is the embedded-file + // key. `Task.File.` is `path` typed, so any + // further segments are property access on that value — + // RFC 0005 uses `{{Task.File.Run.name}}` directly. + // Embedded file names are identifiers (no dots, see + // the name validation below), so the first segment is + // unambiguous. + let file_name = tail.split_once('.').map_or(tail, |(key, _)| key); + if !file_names.contains(file_name) { + errors.add( + script_path, + format!("references undefined embedded file '{file_name}'."), + ); + } + } + } + } + + // Embedded files + if let Some(files) = &script.embedded_files { + let files_path = path_field(script_path, "embeddedFiles"); + if files.is_empty() { + errors.add(&files_path, "must not be empty."); + } + validate_embedded_files(files, &files_path, errors); + } +} + fn validate_action( action: &Action, path: &[PathElement], diff --git a/crates/openjd-model/tests/integration.rs b/crates/openjd-model/tests/integration.rs index c0e49d7e..a6e1cb38 100644 --- a/crates/openjd-model/tests/integration.rs +++ b/crates/openjd-model/tests/integration.rs @@ -71,6 +71,8 @@ mod test_resolved_value_constraints; mod test_scope_library_split; #[path = "integration/test_simple_action_let.rs"] mod test_simple_action_let; +#[path = "integration/test_simple_action_validation.rs"] +mod test_simple_action_validation; #[path = "integration/test_step_dependency_graph.rs"] mod test_step_dependency_graph; #[path = "integration/test_step_param_space_iter.rs"] diff --git a/crates/openjd-model/tests/integration/test_simple_action_validation.rs b/crates/openjd-model/tests/integration/test_simple_action_validation.rs new file mode 100644 index 00000000..5c66f8be --- /dev/null +++ b/crates/openjd-model/tests/integration/test_simple_action_validation.rs @@ -0,0 +1,647 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright by contributors to this project. +// SPDX-License-Identifier: (Apache-2.0 OR MIT) + +//! SimpleAction (§8, FEATURE_BUNDLE_1) validation: the desugared script is +//! checked by exactly the same code as an authored `script`, at both +//! template validation and job creation — and every diagnostic is reported +//! at the field the author wrote (`steps[i] -> bash -> script`, +//! `-> args[k]`, `-> timeout`, ...), never at a node of the desugared form +//! (`steps[i] -> script -> embeddedFiles[0] -> data`). +//! +//! Failure tests assert the full field path + message per the repo's +//! error-message test standard. + +use openjd_expr::path_mapping::PathFormat; +use openjd_model::{create_job, decode_job_template, job, preprocess_job_parameters, CallerLimits}; + +fn yaml_val(s: &str) -> serde_json::Value { + serde_saphyr::from_str(s).unwrap() +} + +const EXTS: &[&str] = &["EXPR", "FEATURE_BUNDLE_1"]; + +fn decode_with( + s: &str, + limits: &CallerLimits, +) -> Result { + decode_job_template(yaml_val(s), Some(EXTS), limits).map_err(|e| e.to_string()) +} + +fn decode_ok(s: &str) { + decode_with(s, &CallerLimits::default()) + .unwrap_or_else(|e| panic!("Expected success for:\n{s}\nGot:\n{e}")); +} + +fn assert_contains(msg: &str, expected: &[&str]) { + for line in expected { + assert!( + msg.contains(line), + "Missing in error output: {line:?}\nGot:\n{msg}" + ); + } +} + +fn check_err(s: &str, expected: &[&str]) { + let msg = decode_with(s, &CallerLimits::default()) + .err() + .unwrap_or_else(|| panic!("Expected error for:\n{s}")); + assert_contains(&msg, expected); +} + +fn check_err_with(s: &str, limits: &CallerLimits, expected: &[&str]) { + let msg = decode_with(s, limits) + .err() + .unwrap_or_else(|| panic!("Expected error for:\n{s}")); + assert_contains(&msg, expected); +} + +/// Decode under default limits (so template validation passes), then run +/// `create_job` under `limits` — the "stricter at submission" pattern. +fn create_with_limits( + template: &str, + params: &[(&str, &str)], + limits: CallerLimits, +) -> Result { + let root = tempfile::TempDir::new().unwrap(); + let dir = root.path().to_str().unwrap(); + let jt = decode_with(template, &CallerLimits::default()) + .expect("template must pass validation under default limits"); + let input: std::collections::HashMap = params + .iter() + .map(|(k, v)| (k.to_string(), openjd_expr::ExprValue::String(v.to_string()))) + .collect(); + let processed = preprocess_job_parameters( + &jt, + &input, + &[], + &openjd_model::PathParameterOptions { + job_template_dir: dir, + current_working_dir: dir, + allow_template_dir_walk_up: true, + path_format: PathFormat::host(), + allow_uri_path_values: true, + }, + ) + .map_err(|e| e.to_string())?; + let mut ctx = jt.default_validation_context(); + ctx.caller_limits = limits; + create_job(&jt, &processed, &ctx).map_err(|e| e.to_string()) +} + +fn data_cap(n: usize) -> CallerLimits { + CallerLimits { + max_resolved_data_len: Some(n), + ..CallerLimits::default() + } +} + +fn arg_cap(n: usize) -> CallerLimits { + CallerLimits { + max_resolved_arg_len: Some(n), + ..CallerLimits::default() + } +} + +// ══════════════════════════════════════════════════════════════ +// Template validation — format-string references (pass 8) +// ══════════════════════════════════════════════════════════════ + +#[test] +fn undefined_variable_in_script_reported_at_sugar_script() { + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "bash": {"script": "echo {{Param.Nope}}"}}] + }"#, + &[ + "steps[0] -> bash -> script:\n\tFailed to parse interpolation expression at [5, 19]. Undefined variable: 'Param.Nope'.", + ], + ); +} + +#[test] +fn undefined_variable_in_args_reported_at_authored_index() { + // The desugared `args` is [, "ok", "{{Param.Nope}}"]; the + // author wrote args[1], so that is what the path must say. + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "bash": {"script": "echo", "args": ["ok", "{{Param.Nope}}"]}}] + }"#, + &[ + "steps[0] -> bash -> args[1]:\n\tFailed to parse interpolation expression at [0, 14]. Undefined variable: 'Param.Nope'.", + ], + ); +} + +#[test] +fn args_index_accounts_for_interpreter_prefix() { + // cmd desugars to ["/C", , ...authored]; powershell to + // ["-File", , ...authored]. Authored args[0] must stay args[0]. + for kind in ["cmd", "powershell"] { + let s = format!( + r#"{{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{{"name": "S", "{kind}": {{"script": "echo", "args": ["{{{{Param.Nope}}}}"]}}}}] + }}"# + ); + check_err( + &s, + &[&format!( + "steps[0] -> {kind} -> args[0]:\n\tFailed to parse interpolation expression at [0, 14]. Undefined variable: 'Param.Nope'." + )], + ); + assert!( + !decode_with(&s, &CallerLimits::default()) + .unwrap_err() + .contains("-> script ->"), + "{kind}: no diagnostic may name the desugared script node" + ); + } +} + +#[test] +fn timeout_and_cancelation_validate_in_template_scope() { + // `timeout`/`notifyPeriodInSeconds` are plain @fmtstring (job-creation + // stage): Session.* is not in scope, exactly as for an authored Action. + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "python": { + "script": "print(1)", + "timeout": "{{Param.T}}", + "cancelation": {"mode": "NOTIFY_THEN_TERMINATE", "notifyPeriodInSeconds": "{{Param.N}}"} + }}] + }"#, + &[ + "steps[0] -> python -> timeout:\n\tFailed to parse interpolation expression at [0, 11]. Undefined variable: 'Param.T'.", + "steps[0] -> python -> cancelation:\n\tFailed to parse interpolation expression at [0, 11]. Undefined variable: 'Param.N'.", + ], + ); +} + +#[test] +fn session_symbol_in_timeout_is_rejected() { + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "node": {"script": "x", "timeout": "{{Session.WorkingDirectory}}"}}] + }"#, + &["steps[0] -> node -> timeout:\n\tFailed to parse interpolation expression at [0, 28]. Undefined variable: 'Session.WorkingDirectory'."], + ); +} + +#[test] +fn let_without_expr_is_rejected_at_sugar_let() { + // Previously the SimpleAction `let` was only examined when EXPR was on, + // so a `let` without EXPR passed `check` and was silently ignored. + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "bash": {"let": ["x = 1"], "script": "echo"}}] + }"#, + &["steps[0] -> bash -> let:\n\t'let' requires the EXPR extension."], + ); +} + +#[test] +fn let_binding_errors_carry_the_binding_index() { + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1", "EXPR"], + "name": "Test", + "steps": [{"name": "S", "bash": {"let": ["x = 1", "y = 2", "x = 3"], "script": "echo"}}] + }"#, + &["steps[0] -> bash -> let[2]:\n\tduplicate name 'x'."], + ); +} + +#[test] +fn comprehension_var_shadowing_let_reported_at_script_and_args() { + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1", "EXPR"], + "name": "Test", + "steps": [{"name": "S", "bash": { + "let": ["x = 1"], + "script": "echo {{ [x for x in [1, 2]] }}", + "args": ["{{ [x for x in [1, 2]] }}"] + }}] + }"#, + &[ + "steps[0] -> bash -> script:\n\tList comprehension variable 'x' shadows a let binding", + "steps[0] -> bash -> args[0]:\n\tList comprehension variable 'x' shadows a let binding", + ], + ); +} + +#[test] +fn complex_expression_without_expr_reported_at_authored_arg() { + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "parameterDefinitions": [{"name": "N", "type": "INT", "default": 1}], + "steps": [{"name": "S", "bash": {"script": "echo", "args": ["{{Param.N + 1}}"]}}] + }"#, + &["steps[0] -> bash -> args[0]:\n\tcomplex expressions require the EXPR extension."], + ); +} + +#[test] +fn well_formed_simple_actions_pass() { + decode_ok( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1", "EXPR"], + "name": "Test", + "parameterDefinitions": [{"name": "N", "type": "INT", "default": 1}], + "steps": [ + {"name": "A", "bash": {"let": ["x = Param.N * 2"], "script": "echo {{x}} {{Session.WorkingDirectory}}", "args": ["{{Param.N}}"], "timeout": "{{Param.N}}"}}, + {"name": "B", "cmd": {"script": "echo %1", "args": ["{{Param.N}}"]}}, + {"name": "C", "powershell": {"script": "Write-Host $args", "args": ["-x", "{{Param.N}}"]}}, + {"name": "D", "python": {"script": "print({{Param.N}})", "cancelation": {"mode": "NOTIFY_THEN_TERMINATE", "notifyPeriodInSeconds": "{{Param.N}}"}}}, + {"name": "E", "node": {"script": "console.log({{Param.N}})"}} + ] + }"#, + ); +} + +#[test] +fn sugar_may_reference_its_own_generated_file() { + // The generated embedded file is `_script`; it is + // in task scope for the sugar's `args`, `script`, and `let` exactly as + // for the desugared form. + decode_ok( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1", "EXPR"], + "name": "Test", + "steps": [{"name": "My Step", "bash": { + "let": ["here = Task.File.My_Step_script"], + "script": "echo {{here}} {{Task.File.My_Step_script.name}}", + "args": ["{{Task.File.My_Step_script}}"] + }}] + }"#, + ); +} + +#[test] +fn undefined_task_file_in_sugar_args_reported_at_authored_paths() { + // Pass 6 reports the dangling reference at the script node, which for + // sugar is the field itself; pass 8 reports the undefined symbol at + // the authored arg. Neither names the desugared form. + let s = r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "python": {"script": "x", "args": ["{{Task.File.Other}}"]}}] + }"#; + check_err( + s, + &[ + "steps[0] -> python:\n\treferences undefined embedded file 'Other'.", + "steps[0] -> python -> args[0]:\n\tFailed to parse interpolation expression at [0, 19]. Undefined variable: 'Task.File.Other'.", + ], + ); + let msg = decode_with(s, &CallerLimits::default()).unwrap_err(); + assert!( + !msg.contains("-> script ->") && !msg.contains("embeddedFiles") && !msg.contains("onRun"), + "no diagnostic may name the desugared form:\n{msg}" + ); +} + +#[test] +fn args_index_for_single_prefix_kinds() { + // bash/python/node: exactly one synthesized arg (the file reference). + for kind in ["bash", "python", "node"] { + let s = format!( + r#"{{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{{"name": "S", "{kind}": {{"script": "x", "args": ["a", "b", "{{{{Param.Nope}}}}"]}}}}] + }}"# + ); + check_err( + &s, + &[&format!( + "steps[0] -> {kind} -> args[2]:\n\tFailed to parse interpolation expression at [0, 14]. Undefined variable: 'Param.Nope'." + )], + ); + } +} + +#[test] +fn script_plus_sugar_reports_only_the_conflict() { + // Pass 7 rejects the combination. The sugar is not additionally + // validated: doing so against a step whose `Task.File.*` come from the + // authored `script` would report the generated file reference as + // undefined — a message naming a node the author never wrote. + let s = r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{ + "name": "S", + "script": {"actions": {"onRun": {"command": "echo"}}}, + "node": {"script": "x"} + }] + }"#; + let msg = decode_with(s, &CallerLimits::default()).unwrap_err(); + assert_contains( + &msg, + &["steps[0] -> node:\n\tcannot have both 'node' and 'script'."], + ); + assert!( + !msg.contains("_script") && msg.starts_with("Model validation error: 1 validation error"), + "only the conflict may be reported:\n{msg}" + ); +} + +#[test] +fn two_sugar_fields_each_validated_at_their_own_path() { + // Pass 7 rejects the pair; each field is still validated in full and + // reported at its own path (they share the generated file name, so + // the task symtab is right for both). + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{ + "name": "S", + "bash": {"script": "{{Param.A}}"}, + "python": {"script": "{{Param.B}}"} + }] + }"#, + &[ + "steps[0]:\n\tcannot have more than one simple action field.", + "steps[0] -> bash -> script:\n\tFailed to parse interpolation expression at [0, 11]. Undefined variable: 'Param.A'.", + "steps[0] -> python -> script:\n\tFailed to parse interpolation expression at [0, 11]. Undefined variable: 'Param.B'.", + ], + ); +} + +#[test] +fn non_ascii_step_name_desugars_and_validates() { + // Used to panic in `desugar` at the generated file reference (the + // sanitizer kept `char::is_alphanumeric` characters the expression + // parser rejects). Now sanitized to ASCII as in the Python reference. + decode_ok( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [ + {"name": "²x", "bash": {"script": "echo"}}, + {"name": "a½", "python": {"script": "x"}}, + {"name": "١", "node": {"script": "x", "args": ["{{Task.File.__script}}"]}} + ] + }"#, + ); +} + +// ══════════════════════════════════════════════════════════════ +// Template validation — parse-time and structural (passes 0/6) +// ══════════════════════════════════════════════════════════════ + +#[test] +fn malformed_script_format_string_fails_at_parse() { + // `script` is a `` (@fmtstring[host]); before, this parsed + // as a plain string, passed `check`, and only failed inside + // `create_job` with no field path at all. Deserialization errors carry + // no field path for authored format-string fields either, so only the + // message is asserted here. + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "bash": {"script": "echo {{ "}}] + }"#, + &["Failed to parse interpolation expression at [5, 8]. Reason: Braces mismatch."], + ); +} + +#[test] +fn empty_script_is_rejected() { + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "bash": {"script": ""}}] + }"#, + &["steps[0] -> bash -> script:\n\tmust not be empty."], + ); +} + +#[test] +fn empty_args_list_is_rejected() { + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "bash": {"script": "echo", "args": []}}] + }"#, + &["steps[0] -> bash -> args:\n\tif provided, must not be empty."], + ); +} + +#[test] +fn literal_timeout_and_notify_period_range_checks() { + // The numeric literal checks an authored Action gets at + // `steps[0] -> script -> actions -> onRun` land on the sugar field. + check_err( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "bash": { + "script": "echo", + "timeout": 0, + "cancelation": {"mode": "NOTIFY_THEN_TERMINATE", "notifyPeriodInSeconds": 700} + }}] + }"#, + &[ + "steps[0] -> bash:\n\tnotifyPeriodInSeconds must not exceed 600.", + "steps[0] -> bash:\n\ttimeout must be > 0.", + ], + ); +} + +#[test] +fn control_characters_in_arg_reported_at_authored_index() { + check_err( + "{ + \"specificationVersion\": \"jobtemplate-2023-09\", + \"extensions\": [\"FEATURE_BUNDLE_1\"], + \"name\": \"Test\", + \"steps\": [{\"name\": \"S\", \"powershell\": {\"script\": \"echo\", \"args\": [\"fine\", \"bad\\u0007\"]}}] + }", + &["steps[0] -> powershell -> args[1]:\n\tcontains control characters."], + ); +} + +// ══════════════════════════════════════════════════════════════ +// Template validation — opt-in resolved-value caps (gate 1) +// ══════════════════════════════════════════════════════════════ + +#[test] +fn static_script_over_data_cap_fails_check() { + // Follow-up item 4's motivating case: a 500-char `bash:` body under + // `max_resolved_data_len: 100` used to pass `check` and fail only at + // `create_job`, at `steps[0] -> script -> embeddedFiles[0] -> data`. + let s = format!( + r#"{{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{{"name": "S", "bash": {{"script": "{}"}}}}] + }}"#, + "A".repeat(500) + ); + check_err_with( + &s, + &data_cap(100), + &["steps[0] -> bash -> script:\n\tis 500 characters, exceeding the maximum of 100."], + ); +} + +#[test] +fn partially_static_script_over_data_cap_fails_check_by_lower_bound() { + let s = r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1", "EXPR"], + "name": "Test", + "steps": [{"name": "S", "bash": {"script": "{{Session.WorkingDirectory}}/{{ 'A' * 200 }}"}}] + }"#; + check_err_with( + s, + &data_cap(100), + &["steps[0] -> bash -> script:\n\tresolves to at least 201 characters, exceeding the maximum of 100."], + ); +} + +#[test] +fn static_arg_over_arg_cap_fails_check_at_authored_index() { + let s = format!( + r#"{{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{{"name": "S", "cmd": {{"script": "echo", "args": ["short", "{}"]}}}}] + }}"#, + "B".repeat(300) + ); + check_err_with( + &s, + &arg_cap(100), + &["steps[0] -> cmd -> args[1]:\n\tis 300 characters, exceeding the maximum of 100."], + ); +} + +#[test] +fn synthesized_file_reference_is_not_measured_against_arg_cap() { + // The generated `{{Task.File.}}` arg is unresolved at every + // client stage (contributes 0 to the bound), and the literal + // interpreter args are short; a tiny cap must not trip on them. + let s = r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "steps": [{"name": "S", "powershell": {"script": "echo"}}] + }"#; + decode_with(s, &arg_cap(10)).expect("only synthesized args; must pass"); +} + +// ══════════════════════════════════════════════════════════════ +// Job creation (gate 2) — same checks, same authored paths +// ══════════════════════════════════════════════════════════════ + +fn param_script_template(kind: &str) -> String { + format!( + r#"{{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "name": "Test", + "parameterDefinitions": [{{"name": "X", "type": "STRING"}}], + "steps": [{{"name": "S", "{kind}": {{"script": "{{{{Param.X}}}}", "args": ["--flag", "{{{{Param.X}}}}"]}}}}] + }}"# + ) +} + +#[test] +fn script_over_data_cap_from_param_fails_at_create_job_at_sugar_path() { + assert_contains( + &create_with_limits( + ¶m_script_template("bash"), + &[("X", &"A".repeat(200))], + data_cap(100), + ) + .expect_err("over-cap data must fail at create_job"), + &["steps[0] -> bash -> script:\n\tresolves to at least 200 characters, exceeding the maximum of 100."], + ); +} + +#[test] +fn arg_over_arg_cap_from_param_fails_at_create_job_at_authored_index() { + // Desugared cmd args: ["/C", , "--flag", "{{Param.X}}"]; the + // author's failing arg is args[1]. + let msg = create_with_limits( + ¶m_script_template("cmd"), + &[("X", &"A".repeat(200))], + arg_cap(100), + ) + .expect_err("over-cap arg must fail at create_job"); + assert_contains( + &msg, + &["steps[0] -> cmd -> args[1]:\n\tresolves to at least 200 characters, exceeding the maximum of 100."], + ); + assert!( + !msg.contains("-> script ->") && !msg.contains("embeddedFiles"), + "no diagnostic may name the desugared form:\n{msg}" + ); +} + +#[test] +fn simple_action_under_caps_creates_job() { + let job = create_with_limits( + ¶m_script_template("python"), + &[("X", "short")], + CallerLimits { + max_resolved_data_len: Some(100), + max_resolved_arg_len: Some(100), + ..CallerLimits::default() + }, + ) + .expect("under-cap SimpleAction must create a job"); + let action = &job.steps[0].script.actions.on_run; + assert_eq!(action.command.raw(), "python"); + let args: Vec<&str> = action + .args + .as_ref() + .unwrap() + .iter() + .map(|a| a.raw()) + .collect(); + assert_eq!(args, ["{{Task.File.S_script}}", "--flag", "{{Param.X}}"]); +} diff --git a/crates/openjd-model/tests/integration/test_template_public_api.rs b/crates/openjd-model/tests/integration/test_template_public_api.rs index 9f9ecf78..384eb571 100644 --- a/crates/openjd-model/tests/integration/test_template_public_api.rs +++ b/crates/openjd-model/tests/integration/test_template_public_api.rs @@ -246,7 +246,7 @@ fn simple_action_sugar_field_access() { let s = &jt.steps[0]; assert!(s.script.is_none()); let bash: &SimpleAction = s.bash.as_ref().unwrap(); - assert_eq!(bash.script, "echo hi"); + assert_eq!(bash.script.raw(), "echo hi"); assert!(s.python.is_none()); } diff --git a/specs/model/job-creation.md b/specs/model/job-creation.md index 50fd16c0..7fcad197 100644 --- a/specs/model/job-creation.md +++ b/specs/model/job-creation.md @@ -207,7 +207,10 @@ budgets template validation and the session runtime apply. and `Step.Name` (per step) into the symbol table 4. Carry forward session/task-scope fields as FormatString (plus action `timeout`/`notifyPeriodInSeconds`, which validate in template scope but - resolve on the worker) + resolve on the worker). A SimpleAction step (§8) is desugared here via + `StepTemplate::resolve_syntax_sugar` — infallible, since the sugar's + `script` is already a parsed `FormatString` — so the job's `Step` always + carries a full `StepScript`. 5. Run the resolved-value checks on the carried-forward fields (next section) 6. Convert environments from template to job types @@ -285,6 +288,13 @@ Failures are `ModelError::ModelValidation` at the same field paths pass 8 uses, e.g. `steps[0] -> script -> actions -> onRun -> args[0]:` / `resolves to at least 100000 characters, exceeding the maximum of 1024.` +For a SimpleAction step (§8) the checks run on the desugared +`StepScript` — the same form pass 8 validated — and the paths are +re-rooted onto the field the author wrote through the same +`SimpleActionKind::remap_desugared_path` table pass 8 uses +(`steps[0] -> bash -> script`, `steps[0] -> cmd -> args[1]`, …; see +`specs/model/validation.md` § Reporting on desugared forms). No +diagnostic from this stage names a synthesized node. Violations accumulate within one scope (a step script, one environment's fields), but the first failing scope stops instantiation — consistent with the fail-fast resolved-value re-checks `create_job` diff --git a/specs/model/public-api.md b/specs/model/public-api.md index 344bd796..8f94ee41 100644 --- a/specs/model/public-api.md +++ b/specs/model/public-api.md @@ -59,7 +59,7 @@ The structural template types — `template::JobTemplate`, `template::StepActions`, `template::CancelationMode`, `template::HostRequirements`, `template::AmountRequirement`, `template::AttributeRequirement`, `template::StepDependency`, -`template::SimpleAction`, `template::Description`, +`template::SimpleAction`, `template::SimpleActionKind`, `template::Description`, `template::ExtensionName`, `template::TaskParameterDefinition` and its 5 per-variant inner struct types (`IntTaskParameterDefinition`, `FloatTaskParameterDefinition`, @@ -90,6 +90,41 @@ instantiation — for example, the `openjd-python` bindings expose typed `template::*` pyclasses so Python tools can introspect job templates. +`template::StepTemplate` additionally exposes the SimpleAction (§8, +FEATURE_BUNDLE_1) surface: + +```rust +impl StepTemplate { + /// Each SimpleAction field the step sets, with its kind, in + /// `SimpleActionKind::ALL` order. + pub fn simple_actions(&self) -> impl Iterator; + pub fn simple_action(&self, kind: SimpleActionKind) -> Option<&SimpleAction>; + /// `script.clone()` when present, otherwise the first SimpleAction + /// desugared; `None` when the step has neither. Infallible — + /// `SimpleAction.script` is already a parsed `FormatString`. + pub fn resolve_syntax_sugar(&self) -> Option; +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum SimpleActionKind { Python, Bash, Cmd, Powershell, Node } + +impl SimpleActionKind { + pub const ALL: [SimpleActionKind; 5]; + pub fn field_name(self) -> &'static str; + pub fn file_extension(self) -> &'static str; + pub fn arg_prefix(self) -> &'static [&'static str]; + pub fn synthetic_arg_count(self) -> usize; + pub fn desugar(self, step_name: &str, sa: &SimpleAction) -> StepScript; + /// Map a validation-error path on the desugared `StepScript` (relative + /// to its `script` node) back onto the authored sugar field. + pub fn remap_desugared_path(self, rel: &[PathElement]) -> Vec; +} +``` + +See `specs/model/template-types.md` § SimpleAction for the desugaring +rules and `specs/model/validation.md` § Reporting on desugared forms for +the path table. + ## Entry Points at the Crate Root ### Parsing + Validation diff --git a/specs/model/template-types.md b/specs/model/template-types.md index 268afb49..6965868e 100644 --- a/specs/model/template-types.md +++ b/specs/model/template-types.md @@ -80,20 +80,56 @@ pub struct StepTemplate { ### SimpleAction (FEATURE_BUNDLE_1) -Syntax sugar that expands into a `StepScript` with an embedded file and `onRun` action. -The `resolve_syntax_sugar()` method performs this expansion. A step must have either `script` -or exactly one simple action field — never both. +Syntax sugar that expands into a `StepScript` with an embedded file and `onRun` action +(Template Schemas §8). A step must have either `script` or exactly one simple action +field — never both. ```rust pub struct SimpleAction { pub let_bindings: Option>, - pub script: String, + /// ``, `@fmtstring[host]` — parsed at deserialization, so + /// malformed `{{ ... }}` fails at parse time like any other format string. + pub script: FormatString, pub args: Option>, pub timeout: Option, pub cancelation: Option, } + +/// Which interpreter key the SimpleAction was written under. +pub enum SimpleActionKind { Python, Bash, Cmd, Powershell, Node } + +impl SimpleActionKind { + pub const ALL: [SimpleActionKind; 5]; // the order resolve_syntax_sugar considers them + pub fn field_name(self) -> &'static str; // "bash", … — also the desugared `command` + pub fn file_extension(self) -> &'static str; // ".sh", ".py", ".bat", ".ps1", ".js" + pub fn arg_prefix(self) -> &'static [&'static str]; // ["/C"] for cmd, ["-File"] for powershell, else [] + pub fn synthetic_arg_count(self) -> usize; // arg_prefix().len() + 1 (the Task.File reference) + pub fn desugar(self, step_name: &str, sa: &SimpleAction) -> StepScript; + pub fn remap_desugared_path(self, rel: &[PathElement]) -> Vec; +} + +impl StepTemplate { + pub fn simple_actions(&self) -> impl Iterator; + pub fn simple_action(&self, kind: SimpleActionKind) -> Option<&SimpleAction>; + /// `script.clone()` if present, else the first SimpleAction desugared; `None` if neither. + pub fn resolve_syntax_sugar(&self) -> Option; +} ``` +`desugar` builds the generated embedded file's name from the step name +(every character outside `[A-Za-z0-9]` → `_`, as in `openjd-model-for-python`; +at most 200 chars; `_`-prefixed if it would start with a digit; suffixed +`_script` — always a valid identifier, so the generated reference always +parses), sets `command` to the interpreter name, +and produces `args = [..., "{{Task.File.}}", ...]`. + +`remap_desugared_path` maps a validation-error path on the desugared +`StepScript` (relative to its `script` node) back onto the sugar field the +author wrote, so a diagnostic never names a node that does not exist in the +template — see `specs/model/validation.md` § Reporting on desugared forms +for the table. Validation (passes 6 and 8) and job creation all validate the +desugared form and report through this remap. + ### StepDependency (§3.2) ```rust diff --git a/specs/model/validation.md b/specs/model/validation.md index a657b1b4..688af79c 100644 --- a/specs/model/validation.md +++ b/specs/model/validation.md @@ -155,6 +155,16 @@ The largest pass. Validates template structure using `EffectiveRules`. Key check - Embedded files: no duplicate names, type must be `TEXT`, valid identifier names, data required; `filename` must be a single safe path component — non-empty, no path separators (`/` or `\`), no null characters, and not `.` or `..` +- SimpleAction fields (`bash`/`python`/`cmd`/`powershell`/`node`, §8; only + when FEATURE_BUNDLE_1 is enabled — otherwise pass 7 rejects the field): + the field is desugared (`SimpleActionKind::desugar`) and the resulting + `StepScript` runs through the *same* script-action and embedded-file + checks above, so the sugar and its expansion can never disagree on + what is valid. Plus one check the desugared form cannot express: an + authored `args: []` is rejected (the desugared list always holds the + generated file reference). See + [Reporting on desugared forms](#reporting-on-desugared-forms) for how + the paths are reported. **Cycle detection:** - Iterative DFS with tri-state marking (Unvisited/Started/Completed) on the step @@ -170,7 +180,9 @@ The largest pass. Validates template structure using `EffectiveRules`. Key check Validates or rejects features gated behind `FEATURE_BUNDLE_1`: - **SimpleAction fields** (bash, python, cmd, powershell, node): Rejected without extension; - mutually exclusive with `script` when enabled + mutually exclusive with `script` when enabled. This pass only gates the + field's presence — its contents are validated by passes 6 and 8 on the + desugared form (see Pass 8 § Step Scripts and SimpleActions). - **`endOfLine` on embedded files**: Rejected without extension; must be `LF`, `CRLF`, or `AUTO` when enabled @@ -209,6 +221,71 @@ session/task scope. `Task.File.*`. With EXPR: adds `Job.Name`, `Step.Name`, `Env.File.*` from step and job environments. +### Step Scripts and SimpleActions + +One helper (`validate_step_script_format_strings`) validates a `StepScript` +in full — script-level `let` bindings (into the task symtab), the +EXPR-gated complex-expression rejection, the `onRun` action's +`command`/`args` in task scope with the opt-in `max_resolved_arg_len` +bound, `timeout`/`cancelation` in template scope, each embedded file's +`data` in task scope with the opt-in `max_resolved_data_len` bound, and +the comprehension-variable rules (against the step's and the script's +`let` names) on `command`, `args`, and embedded `data`. + +An authored `script` runs through it once. Each SimpleAction field the +step sets (§8; only when FEATURE_BUNDLE_1 is enabled and the step has no +`script` — pass 7 rejects both other cases, and validating sugar against +a task symtab whose `Task.File.*` came from an authored `script` would +report the generated file reference as undefined) is desugared and runs +through the **same helper**, so a `bash:` step is validated exactly as +its `script:` equivalent would be — the design goal that job creation +"re-runs exactly the checks pass 8 applies" holds for sugar too. Each +field gets a clone of the task symtab, so its `let` bindings do not leak +into a sibling field. Consequences worth naming, because they changed +behaviour (all of these previously passed `check`): a SimpleAction `let` +without EXPR is now rejected (it was silently ignored); undefined +references in `script`/`args`/`timeout`/`notifyPeriodInSeconds` are +checked at decode; a static `script` over an opted-in +`max_resolved_data_len` fails `check` (it used to surface only at +`create_job`); and the pass-6 structural checks now reach the sugar — +`script: ""`, `args: []`, control characters in `args[k]`, literal +`timeout: 0` / `notifyPeriodInSeconds: 700`, and a dangling +`{{Task.File.Other}}` in `args`. + +#### Reporting on desugared forms + +Every diagnostic must name a node that exists in the template the author +wrote — never `steps[0] -> script -> embeddedFiles[0] -> data` for a step +that has no `script`. Validation of a desugared SimpleAction therefore runs +into a scratch `ValidationErrors` rooted at the synthesized `script` node +(path `[]`), and `ValidationErrors::extend_remapped` re-roots each error +through `SimpleActionKind::remap_desugared_path` before it joins the real +collection (messages and structured detail are carried unchanged): + +| Desugared path (relative to `script`) | Reported path (relative to `steps[i] -> `) | +|------------------------------------------------|--------------------------------------------------| +| `let[j]` | `let[j]` | +| `actions -> onRun -> args` | `args` | +| `actions -> onRun -> args[k]`, `k ≥ offset` | `args[k − offset]` | +| `actions -> onRun -> timeout` | `timeout` | +| `actions -> onRun -> cancelation` | `cancelation` | +| `embeddedFiles[0] -> data` | `script` | +| anything synthesized — `command`, the `onRun` node itself, `args[k]` for `k < offset`, the generated file's `name`/`filename` | the `` field itself | + +`offset` is `SimpleActionKind::synthetic_arg_count()`: the interpreter +prefix (`/C` for `cmd`, `-File` for `powershell`, none otherwise) plus the +generated `{{Task.File.}}` reference — `1` for `bash`/`python`/`node`, +`2` for `cmd`/`powershell`. Pass 6 and job creation use the same remap, so +a violation is reported at the same authored path whichever stage finds +it (a dangling `Task.File.*` reference is still reported by both pass 6 — +at the field, as the script node — and pass 8 — at the arg — exactly as +for an authored `script`). + +`SimpleAction.script` is a `FormatString` (the spec types it ``, +`@fmtstring[host]`), so malformed `{{ ... }}` syntax fails at parse time +like every other format-string field; it no longer survives to job +creation. + ### Let Binding Validation Let bindings are validated with these rules: