From 03ed063451d774195d9bcaf823531d740aa19771 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Tue, 25 Aug 2026 13:31:19 -0700 Subject: [PATCH 1/2] fix: Allow an environment to define only onExit An environment script that defined onExit without onEnter was rejected during template validation with "onEnter is required." This blocked environments whose only work is cleanup, including queue environments that tear down state established elsewhere in the session. Apply the same "at least one action" rule regardless of whether WRAP_ACTIONS is enabled, so either ordinary action alone is sufficient. The error message keeps naming the wrap hooks only when the extension is enabled, since they are not a valid choice otherwise. An actions object that defines no action is still rejected, and the all-or-nothing wrap-hook rule in wrap_actions.rs is unchanged. The session runtime already handles an absent onEnter. enter_environment guards on the action being present, and the environment script runner treats a missing action as a successful no-op, which is the path a variables-only environment already takes. This matches openjd-model-for-python PR 338 and the wiki correction in openjd-specifications PR 178. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../template/validate_v2023_09/structure.rs | 39 ++++++++----------- .../integration/test_actions_and_steps.rs | 9 ++--- .../integration/test_environment_template.rs | 33 +++++++++++++--- 3 files changed, 48 insertions(+), 33 deletions(-) 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 219a7330..6067b3b0 100644 --- a/crates/openjd-model/src/template/validate_v2023_09/structure.rs +++ b/crates/openjd-model/src/template/validate_v2023_09/structure.rs @@ -389,29 +389,22 @@ pub fn validate_single_environment( if let Some(script) = &env.script { let script_path = path_field(path, "script"); let actions_path = path_field(&script_path, "actions"); - // Base 2023-09 requires `onEnter` whenever a `script` is present. - // RFC 0008 relaxes this: when `WRAP_ACTIONS` is enabled, an env may - // define only wrap hooks (or any single action) without a standalone - // `onEnter`. Concretely, require at least one of the five known - // actions to be present so we don't accept an empty `actions: {}`. - if rules.wrap_actions_enabled { - // RFC 0008: an env may define only wrap hooks (or any single - // action) without a standalone `onEnter`. Require at least one of - // the five known actions so an empty `actions: {}` is still - // rejected. The all-or-nothing wrap-hook rule (defining any wrap - // hook requires all three) is enforced separately in - // `wrap_actions.rs`, not here. - if !script.actions.has_any_action() { - errors.add( - &actions_path, - "must define at least one of onEnter or onExit, or the complete set of wrap hooks (onWrapEnvEnter, onWrapTaskRun, and onWrapEnvExit together).", - ); - } - } else if script.actions.on_enter.is_none() { - // Preserve the original wording when the extension is not enabled - // so pre-RFC error messages don't change. `on_enter.is_none()` - // also covers the empty-`actions` case. - errors.add(&actions_path, "onEnter is required."); + // An environment script must define at least one action. Either + // `onEnter` or `onExit` alone satisfies this, so an environment whose + // only work is cleanup is valid. With `WRAP_ACTIONS` a complete set of + // wrap hooks also satisfies it. Requiring at least one of the five + // known actions is what rejects an empty `actions: {}`. The + // all-or-nothing wrap-hook rule (defining any wrap hook requires all + // three) is enforced separately in `wrap_actions.rs`, not here. + if !script.actions.has_any_action() { + errors.add( + &actions_path, + if rules.wrap_actions_enabled { + "must define at least one of onEnter or onExit, or the complete set of wrap hooks (onWrapEnvEnter, onWrapTaskRun, and onWrapEnvExit together)." + } else { + "must define at least one of onEnter or onExit." + }, + ); } for (name, action) in script.actions.iter_named() { validate_action( diff --git a/crates/openjd-model/tests/integration/test_actions_and_steps.rs b/crates/openjd-model/tests/integration/test_actions_and_steps.rs index 117b7bc2..28810b43 100644 --- a/crates/openjd-model/tests/integration/test_actions_and_steps.rs +++ b/crates/openjd-model/tests/integration/test_actions_and_steps.rs @@ -296,7 +296,7 @@ fn test_env_actions_empty() { }"#; check_err( s, - &["jobEnvironments[0] -> script -> actions:\n\tonEnter is required."], + &["jobEnvironments[0] -> script -> actions:\n\tmust define at least one of onEnter or onExit."], ); } @@ -599,10 +599,9 @@ fn test_env_action_on_exit() { "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "foo"}}}}], "jobEnvironments": [{"name": "E", "script": {"actions": {"onExit": {"command": "foo"}}}}] }"#; - check_err( - s, - &["jobEnvironments[0] -> script -> actions:\n\tonEnter is required."], - ); + // Either ordinary action alone is sufficient, so a cleanup-only + // environment is valid. + decode_ok(s); } #[test] diff --git a/crates/openjd-model/tests/integration/test_environment_template.rs b/crates/openjd-model/tests/integration/test_environment_template.rs index a5efd000..192b9476 100644 --- a/crates/openjd-model/tests/integration/test_environment_template.rs +++ b/crates/openjd-model/tests/integration/test_environment_template.rs @@ -744,18 +744,41 @@ fn test_env_name_513_chars_with_extension_fails() { } // ══════════════════════════════════════════════════════════════ -// Bug fix: onEnter is required per spec §4.3 +// Environment actions: at least one of onEnter or onExit (spec §4.3) // ══════════════════════════════════════════════════════════════ #[test] -fn test_env_actions_on_enter_required() { - // onExit alone should fail — onEnter is required - check_env_err( +fn test_env_actions_on_exit_only_is_valid() { + // onExit alone is sufficient: a queue environment whose only work is + // cleanup does not need an onEnter. + decode_ok( r#"{ "specificationVersion": "environment-2023-09", "environment": {"name": "Foo", "script": {"actions": {"onExit": {"command": "cleanup"}}}} }"#, - &["environment -> script -> actions:\n\tonEnter is required."], + ); +} + +#[test] +fn test_env_actions_on_enter_only_is_valid() { + decode_ok( + r#"{ + "specificationVersion": "environment-2023-09", + "environment": {"name": "Foo", "script": {"actions": {"onEnter": {"command": "setup"}}}} + }"#, + ); +} + +#[test] +fn test_env_actions_empty_is_rejected() { + // Removing the onEnter requirement must not make an empty actions object + // acceptable. + check_env_err( + r#"{ + "specificationVersion": "environment-2023-09", + "environment": {"name": "Foo", "script": {"actions": {}}} + }"#, + &["environment -> script -> actions:\n\tmust define at least one of onEnter or onExit."], ); } From 585184c3d696dc068fe86333580577d2b9a55ccf Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:18:31 -0700 Subject: [PATCH 2/2] docs: Correct the wrap_actions_enabled rule description and cover its wording Address review feedback on the at-least-one-action change. The doc comment on EffectiveRules::wrap_actions_enabled still said onEnter is required when the extension is absent, which the previous commit made untrue. Describe what the flag now decides: whether the wrap hooks count as an acceptable choice, and which wording the error uses. A future reader could otherwise reintroduce the removed branch to match the comment. Add the missing test for the WRAP_ACTIONS wording. Only the plain arm was asserted, so swapping the two message arms went undetected. The new test asserts the full error text for an empty actions object with the extension enabled, and that swap now fails three tests. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../src/template/validate_v2023_09/mod.rs | 11 ++++---- .../tests/integration/test_wrap_actions.rs | 25 +++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/crates/openjd-model/src/template/validate_v2023_09/mod.rs b/crates/openjd-model/src/template/validate_v2023_09/mod.rs index 99a9daf6..1b0ec61e 100644 --- a/crates/openjd-model/src/template/validate_v2023_09/mod.rs +++ b/crates/openjd-model/src/template/validate_v2023_09/mod.rs @@ -99,11 +99,12 @@ pub struct EffectiveRules { pub allowed_job_param_types: std::collections::HashSet, pub allowed_task_param_types: std::collections::HashSet, pub allow_fmtstring_in_numeric_fields: bool, - /// When the `WRAP_ACTIONS` extension is enabled, an environment script - /// is valid if it defines any of `onEnter`, `onExit`, or one of the - /// RFC 0008 wrap hooks (`onWrapEnvEnter`, `onWrapTaskRun`, `onWrapEnvExit`). - /// Without the extension, the base 2023-09 rule applies: - /// `onEnter` is required whenever `script` is present. + /// An environment script must define at least one action whether or not + /// this flag is set, and either `onEnter` or `onExit` alone satisfies + /// that. The flag decides two things: whether the RFC 0008 wrap hooks + /// (`onWrapEnvEnter`, `onWrapTaskRun`, `onWrapEnvExit`) also count as an + /// acceptable choice, and which wording the error uses when no action is + /// defined. It does not make `onEnter` required. pub wrap_actions_enabled: bool, } diff --git a/crates/openjd-model/tests/integration/test_wrap_actions.rs b/crates/openjd-model/tests/integration/test_wrap_actions.rs index a40f8897..3a708a57 100644 --- a/crates/openjd-model/tests/integration/test_wrap_actions.rs +++ b/crates/openjd-model/tests/integration/test_wrap_actions.rs @@ -713,6 +713,31 @@ fn defining_zero_wrap_hooks_accepted() { ); } +// ════════════════════════════════════════════════════════════════════ +// At-least-one-action rule, WRAP_ACTIONS wording +// +// An environment script must define at least one action. With +// WRAP_ACTIONS enabled the error names the wrap hooks as a third +// acceptable choice alongside onEnter and onExit. The wording without +// the extension is covered in test_environment_template.rs. +// ════════════════════════════════════════════════════════════════════ + +#[test] +fn empty_env_actions_rejected_with_wrap_hooks_named() { + expect_env_err( + r#"{ + "specificationVersion": "environment-2023-09", + "extensions": ["WRAP_ACTIONS", "EXPR"], + "environment": { + "name": "Empty", + "script": {"actions": {}} + } + }"#, + WRAP_EXTS, + &["environment -> script -> actions:\n\tmust define at least one of onEnter or onExit, or the complete set of wrap hooks (onWrapEnvEnter, onWrapTaskRun, and onWrapEnvExit together)."], + ); +} + // ════════════════════════════════════════════════════════════════════ // EXPR prerequisite (RFC 0008) //