From 9266ae49d4e0db7940df9aef5bbc5b9a634e94ae Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:38:24 -0700 Subject: [PATCH] fix(model): Accept Task.File property access RFC 0005 declares `Task.File.` as `path` typed and shows property access on one directly inside a format string: echo "Script: {{Task.File.Run.name}}" The step-script validator rejected that form. `expression_names()` returns the whole bare dotted lookup (`Task.File.Run.name`), and the validator stripped only the `Task.File.` prefix, then looked the entire remaining tail up as an embedded-file key. `.name` was folded into the filename, so a template using the form the RFC documents failed validation with "references undefined embedded file 'Run.name'" -- even though the runtime resolves the expression correctly and the environment-template path accepts the equivalent `Env.File..name`. Only the first dotted segment is the embedded-file key; the rest is property access on the resulting path value. Embedded file names are identifiers and cannot contain a dot, so the split is unambiguous. Undefined embedded files are still rejected, and the error now names the file key ('Missing') rather than the property tail ('Missing.name'). Found while reviewing RFC 0008 wrap-action support across the two implementations; covered by a new conformance fixture in openjd-specifications#156. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../template/validate_v2023_09/structure.rs | 10 +- .../tests/integration/test_embedded.rs | 151 ++++++++++++++++++ 2 files changed, 160 insertions(+), 1 deletion(-) 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 0c448222..4f951dcd 100644 --- a/crates/openjd-model/src/template/validate_v2023_09/structure.rs +++ b/crates/openjd-model/src/template/validate_v2023_09/structure.rs @@ -292,7 +292,15 @@ pub fn validate_structure( }; for fs in &all_fs { for name in fs.expression_names() { - if let Some(file_name) = name.strip_prefix("Task.File.") { + 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, diff --git a/crates/openjd-model/tests/integration/test_embedded.rs b/crates/openjd-model/tests/integration/test_embedded.rs index 5492e20e..95f66108 100644 --- a/crates/openjd-model/tests/integration/test_embedded.rs +++ b/crates/openjd-model/tests/integration/test_embedded.rs @@ -8,6 +8,9 @@ //! test_environment_template.rs and test_actions_and_steps.rs. //! Uses job templates for limit enforcement (filename length) since //! the Rust crate enforces limits in the job template validation path. +//! +//! Also covers validation of `Task.File.` *references* from a step +//! script's action fields back to the embedded files it declares. use openjd_model::CallerLimits; use openjd_model::{decode_environment_template, decode_job_template}; @@ -188,3 +191,151 @@ fn filename_too_long_job() { r#"{{"name": "Foo", "type": "TEXT", "data": "hello", "filename": "{name}"}}"# )); } +// ══════════════════════════════════════════════════════════════ +// Task.File. reference validation +// +// RFC 0005 declares `Task.File.` as `path` typed and shows property +// access on one directly inside a format string: +// +// echo "Script: {{Task.File.Run.name}}" +// +// The embedded-file key is only the FIRST dotted segment after `Task.File.`; +// any further segments are property access on the resulting path value. A +// validator that looks the whole dotted tail up as an embedded-file key +// rejects the form the RFC itself uses. +// ══════════════════════════════════════════════════════════════ + +/// Job template declaring one embedded file named `file_name`, with `command` +/// and the single onRun arg supplied separately, so a test can vary the +/// `Task.File.*` reference — and its position — independently of the embedded +/// file that is actually declared. Both fields are scanned by the validator. +fn job_with_command_and_arg(file_name: &str, command: &str, arg: &str) -> serde_json::Value { + yaml_val(&format!( + r#"{{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "Test", + "steps": [{{ + "name": "S", + "script": {{ + "embeddedFiles": [{{"name": "{file_name}", "type": "TEXT", "filename": "run.txt", "data": "contents"}}], + "actions": {{"onRun": {{"command": "{command}", "args": ["{arg}"]}}}} + }} + }}] + }}"# + )) +} + +fn decode_template(v: serde_json::Value) -> Result<(), String> { + decode_job_template(v, Some(&["EXPR"]), &CallerLimits::default()) + .map(|_| ()) + .map_err(|e| e.to_string()) +} + +/// Reference placed in the onRun `args`. +fn decode_with_arg(file_name: &str, arg: &str) -> Result<(), String> { + decode_template(job_with_command_and_arg(file_name, "foo", arg)) +} + +/// Reference placed in the onRun `command`. +fn decode_with_command(file_name: &str, command: &str) -> Result<(), String> { + decode_template(job_with_command_and_arg(file_name, command, "arg")) +} + +#[test] +fn task_file_direct_reference_accepted() { + // Baseline: the bare reference with no property access. + decode_with_arg("Run", "{{Task.File.Run}}").expect("bare Task.File. must be accepted"); +} + +#[test] +fn task_file_direct_property_access_accepted() { + // The exact form RFC 0005 uses. `.name` is a property of the path value, + // not part of the embedded-file key. + decode_with_arg("Run", "{{Task.File.Run.name}}") + .expect("direct .name property access on Task.File. must be accepted"); +} + +#[test] +fn task_file_direct_property_access_suffix_accepted() { + decode_with_arg("Run", "{{Task.File.Run.suffix}}") + .expect("direct .suffix property access on Task.File. must be accepted"); +} + +#[test] +fn task_file_direct_property_access_parent_accepted() { + decode_with_arg("Run", "{{Task.File.Run.parent}}") + .expect("direct .parent property access on Task.File. must be accepted"); +} + +#[test] +fn task_file_nested_property_chain_accepted() { + // Only the FIRST segment is the file key — not "all but the last". A + // chained property access must not be mistaken for a longer file key. + decode_with_arg("Run", "{{Task.File.Run.parent.name}}") + .expect("chained property access on Task.File. must be accepted"); +} + +#[test] +fn task_file_property_access_in_command_accepted() { + // The validator scans onRun.command as well as args. + decode_with_command("Run", "{{Task.File.Run.name}}") + .expect("property access in onRun.command must be accepted"); +} + +// ── Negative controls: an undefined embedded file must still be rejected ── +// +// Note: these templates also fail the format-string symbol resolution, which +// independently reports `Undefined variable`. So the load-bearing part of each +// assertion below is the *message content*, not merely that decoding failed. + +#[test] +fn task_file_unknown_name_rejected() { + let msg = decode_with_arg("Run", "{{Task.File.Missing}}").expect_err("expected decode failure"); + assert!( + msg.contains("steps[0] -> script:\n\treferences undefined embedded file 'Missing'."), + "Expected undefined-embedded-file error at steps[0] -> script naming 'Missing', got: {msg}" + ); +} + +#[test] +fn task_file_unknown_name_with_property_rejected() { + // Still an undefined embedded file, and the error must name the file key + // ('Missing') rather than the property-access tail ('Missing.name'). + let msg = + decode_with_arg("Run", "{{Task.File.Missing.name}}").expect_err("expected decode failure"); + assert!( + msg.contains("steps[0] -> script:\n\treferences undefined embedded file 'Missing'."), + "Expected error to name the embedded-file key 'Missing', got: {msg}" + ); + // The property tail must not be folded into the embedded-file key. Only + // this specific message is constrained: the format-string validator also + // reports the whole dotted path as an undefined variable, which is correct. + assert!( + !msg.contains("undefined embedded file 'Missing.name'"), + "Property tail must not be treated as part of the file key, got: {msg}" + ); +} + +#[test] +fn task_file_unknown_name_in_command_rejected() { + let msg = decode_with_command("Run", "{{Task.File.Missing.name}}") + .expect_err("expected decode failure"); + assert!( + msg.contains("steps[0] -> script:\n\treferences undefined embedded file 'Missing'."), + "Expected undefined-embedded-file error for a reference in command, got: {msg}" + ); +} + +#[test] +fn embedded_file_name_with_dot_rejected() { + // Load-bearing invariant for the first-segment split above: an embedded + // file name is an identifier, so it can never itself contain a dot. If + // this ever changed, `Task.File.a.b` would become ambiguous. + let msg = decode_with_arg("My.File", "{{Task.File.Run}}") + .expect_err("dotted embedded file name must be rejected"); + assert!( + msg.contains("'My.File' is not a valid identifier"), + "Expected invalid-identifier error for a dotted embedded file name, got: {msg}" + ); +}