From ae83e51264da4f146e747a1c423235391533c7e3 Mon Sep 17 00:00:00 2001 From: Sean Tang <171081544+seant-aws@users.noreply.github.com> Date: Tue, 11 Aug 2026 22:46:51 +0000 Subject: [PATCH 1/2] fix(sessions): resolve Env.File.* inside wrap action hooks A wrap environment's embedded files were never registered as `Env.File.*` symbols in the resolution scope of its wrap hooks, so a hook referencing `{{Env.File.}}` failed with `Undefined variable`. This affected all three hooks -- onWrapEnvEnter, onWrapTaskRun and onWrapEnvExit -- since none of them consulted the wrap environment's `embedded_files`. Only inline wrap scripts worked, which is why the gap went unnoticed: every existing wrap test declared `embedded_files: None`. `EmbeddedFiles::allocate_file_paths` both allocates on-disk paths and defines the symbols in one pass, so there was no way to reuse an allocation across invocations. This adds `register_file_paths`, which defines the symbols for already-allocated records without allocating paths or creating files, and a per-wrap-environment cache on `Session` keyed by the environment identifier. Paths are allocated on the first hook invocation and reused thereafter, so `Env.File.*` paths stay stable across tasks and unnamed embedded files do not accumulate one temporary file per task. Contents are rewritten per invocation so each invocation starts from the authored content. The cache is dropped when the wrap environment itself exits; the files live in the session files directory and are cleaned up with it. The registration is driven from the three hook dispatch sites rather than from `seed_wrapped_action_symbols`, which would otherwise be the single shared place to put it. That function evaluates the wrap environment's `let` bindings and replaces the symbol table with the result, so the required order is: register file paths, then seed, then write contents. Registering first is what allows a wrap environment's `let` binding to reference `{{Env.File.*}}`; writing last is what allows embedded file `data` to resolve against the post-`let` symbol table. This matches the ordering already used by `build_wrapped_inner_scope`. Driving it from inside the seed function would also require threading the session's files directory, session id and user through a function that has no `&self`, and whose `WrapEnvironmentScope` argument does not carry `embedded_files`. Tests cover `Env.File` in a wrap-only environment (no `onEnter`), in an environment that also has `onEnter`, in the enter and exit hooks, in a `let` binding, with multiple embedded files, and when the hook does not reference the files at all. Two path-stability tests exist deliberately. The named-file case is insufficient on its own: a named embedded file always resolves to `/`, so its path is identical whether or not the allocation is cached. Mutation testing confirmed this -- forcing the cache-miss branch left the named-file test passing while the unnamed-file test failed with two different generated filenames. Only an unnamed file, whose name is generated at allocation time, can detect a regression in the caching behaviour. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com> --- crates/openjd-sessions/src/embedded_files.rs | 39 +- crates/openjd-sessions/src/session.rs | 178 ++++- .../tests/integration/test_wrap_actions.rs | 673 ++++++++++++++++++ specs/sessions/embedded-files.md | 51 ++ specs/sessions/session.md | 6 +- 5 files changed, 935 insertions(+), 12 deletions(-) diff --git a/crates/openjd-sessions/src/embedded_files.rs b/crates/openjd-sessions/src/embedded_files.rs index 2044e6ba..485808ab 100644 --- a/crates/openjd-sessions/src/embedded_files.rs +++ b/crates/openjd-sessions/src/embedded_files.rs @@ -213,7 +213,7 @@ fn validate_resolved_filename(resolved: &str) -> Result<(), String> { } struct FileRecord { - _symbol: String, + symbol: String, filename: PathBuf, file: EmbeddedFile, } @@ -311,7 +311,7 @@ impl EmbeddedFiles { ) .map_err(|e| SessionError::Runtime(format!("Failed to set {symbol}: {e}")))?; self.records.push(FileRecord { - _symbol: symbol, + symbol, filename, file: file.clone(), }); @@ -319,6 +319,41 @@ impl EmbeddedFiles { Ok(()) } + /// Re-register previously allocated file paths into a symbol table. + /// + /// This is used when a wrap environment's embedded file paths have already + /// been allocated (on the first wrap-hook invocation) and need to be made + /// visible in subsequent wrap-hook scopes without re-allocating paths or + /// creating files on disk. Contents are NOT written — call + /// `write_file_contents` separately after the scope is fully built. + pub(crate) fn register_file_paths(&self, symtab: &mut SymbolTable) -> Result<(), SessionError> { + let scope_name = match self.scope { + EmbeddedFilesScope::Step => "Task", + EmbeddedFilesScope::Env => "Environment", + }; + session_log!( + info, + &self.session_id, + LogContent::FILE_PATH, + "Reusing embedded file paths for {} scope.", + scope_name + ); + for record in &self.records { + symtab + .set( + &record.symbol, + ExprValue::new_path( + record.filename.to_string_lossy().to_string(), + PathFormat::host(), + ), + ) + .map_err(|e| { + SessionError::Runtime(format!("Failed to set {}: {e}", record.symbol)) + })?; + } + Ok(()) + } + pub fn write_file_contents( &self, symtab: &SymbolTable, diff --git a/crates/openjd-sessions/src/session.rs b/crates/openjd-sessions/src/session.rs index c4e79ef2..decde4ae 100644 --- a/crates/openjd-sessions/src/session.rs +++ b/crates/openjd-sessions/src/session.rs @@ -474,6 +474,12 @@ pub struct Session { // Environment tracking environments: HashMap, environments_entered: Vec, + /// Cached embedded-file records for wrap environments. Paths are allocated + /// once per wrap environment and reused for every subsequent wrap-hook + /// invocation so `Env.File.*` paths stay stable across tasks and unnamed + /// files do not accumulate on disk; contents are re-written per invocation + /// so each invocation starts from the authored content. + wrap_env_file_records: HashMap, // Env var tracking env_vars: HashMap, process_env: HashMap, @@ -521,6 +527,7 @@ impl Session { _files_dir: None, environments: HashMap::new(), environments_entered: Vec::new(), + wrap_env_file_records: HashMap::new(), env_vars: HashMap::new(), process_env: HashMap::new(), created_env_vars: HashMap::new(), @@ -724,6 +731,7 @@ impl Session { _files_dir: Some(files_dir), environments: HashMap::new(), environments_entered: Vec::new(), + wrap_env_file_records: HashMap::new(), env_vars: HashMap::new(), process_env, created_env_vars: HashMap::new(), @@ -1194,6 +1202,19 @@ impl Session { let lib = self.library.clone(); if let Some((wrap_env, _)) = wrap_action.as_ref() { + // Register wrap env's embedded file paths BEFORE seed so the + // wrap env's let bindings can reference Env.File.*. + let wrap_env_id = self.wrap_env_id_excluding(&identifier).cloned(); + if let Some(ref wid) = wrap_env_id { + let files = self + .environments + .get(wid) + .and_then(|e| e.script.as_ref()) + .and_then(|s| s.embedded_files.clone()); + self.ensure_wrap_env_files(wid, files.as_deref(), &mut action_symtab) + .map_err(|e| self.fail_action_setup(e))?; + } + // The wrapped onEnter resolves against the INNER env's own // scope (its embedded files and lets) — the same scope // `runner.enter` would have built had the action run @@ -1221,6 +1242,13 @@ impl Session { "onEnter", ) .map_err(|e| self.fail_action_setup(e))?; + + // Write wrap env file contents AFTER seed (so data resolves + // against the post-lets symbol table). + if let Some(ref wid) = wrap_env_id { + self.write_wrap_env_file_contents(wid, &action_symtab, Some(&lib)) + .map_err(|e| self.fail_action_setup(e))?; + } } // The effective action is now resolved (wrap hook or the env's @@ -1440,6 +1468,19 @@ impl Session { let lib = self.library.clone(); if let Some((wrap_env, _)) = wrap_action.as_ref() { + // Register wrap env's embedded file paths BEFORE seed so the + // wrap env's let bindings can reference Env.File.*. + let wrap_env_id = self.active_wrap_env_id().cloned(); + if let Some(ref wid) = wrap_env_id { + let files = self + .environments + .get(wid) + .and_then(|e| e.script.as_ref()) + .and_then(|s| s.embedded_files.clone()); + self.ensure_wrap_env_files(wid, files.as_deref(), &mut action_symtab) + .map_err(|e| self.fail_action_setup(e))?; + } + // See the onEnter path: the wrapped onExit resolves against // the INNER env's own scope. let inner_symtab = self @@ -1464,6 +1505,13 @@ impl Session { "onExit", ) .map_err(|e| self.fail_action_setup(e))?; + + // Write wrap env file contents AFTER seed (so data resolves + // against the post-lets symbol table). + if let Some(ref wid) = wrap_env_id { + self.write_wrap_env_file_contents(wid, &action_symtab, Some(&lib)) + .map_err(|e| self.fail_action_setup(e))?; + } } // See the onEnter path: record the effective action's declared @@ -1581,6 +1629,13 @@ impl Session { String::new() }; + // Evict the wrap-env file cache when the wrap environment itself is + // exited. The on-disk files are intentionally NOT deleted — they live + // in the session files directory and are cleaned up with it. + if env_has_any_wrap_hook(&env) { + self.wrap_env_file_records.remove(identifier); + } + Ok(output) } @@ -1632,15 +1687,37 @@ impl Session { // step script runs exactly as before — this keeps the non-WRAP_ACTIONS // path a zero-cost addition. // - // Scope note: this pass does NOT re-materialize the wrap environment's - // embedded files. Wrap actions that reference `{{Env.File.*}}` will - // see only the names registered when the wrap env was entered, which - // are not persisted across action runs. Inline wrap scripts - // (`command: bash, args: ["-c", "..."]`) work without this. Re-running - // `allocate_file_paths` against the wrap env's embedded_files at task - // dispatch time is the follow-up to enable `Env.File.*` inside wrap - // hooks end-to-end. + // Wrap environment embedded files: paths are allocated once per wrap + // env (on the first hook invocation that triggers a cache miss) and + // reused for every subsequent invocation. File contents are re-written + // per invocation so each hook execution starts from the authored data. + // Registration happens BEFORE seed_wrapped_action_symbols so the wrap + // env's let bindings can reference `{{Env.File.*}}`; writing happens + // AFTER so file data resolves against the post-lets symbol table. let lib = self.library.clone(); + + // Hoist wrap-env identity and embedded files BEFORE the closure that + // borrows `self` immutably, so we can call `&mut self` methods for + // the embedded-file cache. + let wrap_env_id_and_files: Option<( + EnvironmentIdentifier, + Option>, + )> = self.active_wrap_env_id().and_then(|id| { + let env = self.environments.get(id)?; + if env.script.as_ref()?.actions.on_wrap_task_run.is_some() { + let files = env.script.as_ref().and_then(|s| s.embedded_files.clone()); + Some((id.clone(), files)) + } else { + None + } + }); + + // Register wrap env file paths BEFORE seed (so lets can reference Env.File.*). + if let Some((ref wrap_id, ref files)) = wrap_env_id_and_files { + self.ensure_wrap_env_files(wrap_id, files.as_deref(), &mut action_symtab) + .map_err(|e| self.fail_action_setup(e))?; + } + let wrap_action: Option = self .active_wrap_env() .and_then(|wrap_env| { @@ -1680,6 +1757,12 @@ impl Session { .transpose() .map_err(|e| self.fail_action_setup(e))?; + // Write wrap env file contents AFTER seed (so data resolves against post-lets symtab). + if let Some((ref wrap_id, _)) = wrap_env_id_and_files { + self.write_wrap_env_file_contents(wrap_id, &action_symtab, Some(&lib)) + .map_err(|e| self.fail_action_setup(e))?; + } + // Box large locals so they live on the heap instead of inflating // this async fn's state machine. Without this, the combined future // (run_task → drive_action → select!) exceeds Windows' default @@ -2515,6 +2598,85 @@ impl Session { } None } + + /// Like `wrap_env_excluding` but returns the identifier instead of a + /// reference to the environment, avoiding borrow conflicts when `&mut self` + /// is needed after the lookup. + fn wrap_env_id_excluding(&self, self_id: &str) -> Option<&EnvironmentIdentifier> { + for id in self.environments_entered.iter().rev() { + if id == self_id { + continue; + } + if let Some(env) = self.environments.get(id) { + if env_has_any_wrap_hook(env) { + return Some(id); + } + } + } + None + } + + /// Return the `EnvironmentIdentifier` of the active wrap environment, if + /// one exists. Like `active_wrap_env` but returns the id rather than a + /// reference, avoiding borrow conflicts when mutation is needed. + fn active_wrap_env_id(&self) -> Option<&EnvironmentIdentifier> { + for id in self.environments_entered.iter().rev() { + if let Some(env) = self.environments.get(id) { + if env_has_any_wrap_hook(env) { + return Some(id); + } + } + } + None + } + + /// Ensure the wrap environment's embedded file paths are allocated and + /// registered in `symtab`. On the first call for a given wrap env + /// (cache miss), allocates paths and creates the cache entry. On + /// subsequent calls (cache hit), re-registers the previously allocated + /// paths without allocating new ones. + /// + /// No-op when `files` is `None` or empty. + fn ensure_wrap_env_files( + &mut self, + wrap_env_id: &EnvironmentIdentifier, + files: Option<&[openjd_model::job::EmbeddedFile]>, + symtab: &mut SymbolTable, + ) -> Result<(), SessionError> { + let files = match files { + Some(f) if !f.is_empty() => f, + _ => return Ok(()), + }; + if let Some(cached) = self.wrap_env_file_records.get(wrap_env_id) { + // Cache hit: re-register paths without allocating. + cached.register_file_paths(symtab)?; + } else { + // Cache miss: allocate paths and insert into cache. + let mut ef = EmbeddedFiles::new( + EmbeddedFilesScope::Env, + self.files_directory.clone(), + &self.session_id, + ) + .with_user(self.cross_user.user.clone()); + ef.allocate_file_paths(files, symtab)?; + self.wrap_env_file_records.insert(wrap_env_id.clone(), ef); + } + Ok(()) + } + + /// Write the cached wrap environment's embedded file contents into the + /// symtab's resolved scope. No-op if no cache entry exists for the id. + fn write_wrap_env_file_contents( + &self, + wrap_env_id: &EnvironmentIdentifier, + symtab: &SymbolTable, + lib: Option<&FunctionLibrary>, + ) -> Result<(), SessionError> { + if let Some(cached) = self.wrap_env_file_records.get(wrap_env_id) { + cached.write_file_contents(symtab, lib)?; + } + Ok(()) + } } /// Returns true iff the environment defines any of the three wrap hooks diff --git a/crates/openjd-sessions/tests/integration/test_wrap_actions.rs b/crates/openjd-sessions/tests/integration/test_wrap_actions.rs index a64f6697..5a8ea3fa 100644 --- a/crates/openjd-sessions/tests/integration/test_wrap_actions.rs +++ b/crates/openjd-sessions/tests/integration/test_wrap_actions.rs @@ -1185,3 +1185,676 @@ async fn run_wrap_action_applies_default_timeout_when_action_has_none() { "timeout must fire at the default, not wait for the subprocess" ); } + +// ──────────────────────────────────────────────────────────────────── +// Wrap environment embedded files (Env.File.* in wrap hooks) +// ──────────────────────────────────────────────────────────────────── + +/// Helper: build a wrap environment with embedded files and optional on_enter. +fn wrap_env_with_files( + name: &str, + on_enter: Option, + on_wrap_env_enter: Option, + on_wrap_task_run: Option, + on_wrap_env_exit: Option, + embedded_files: Vec, +) -> Environment { + Environment { + name: name.to_string(), + description: None, + script: Some(EnvironmentScript { + let_bindings: None, + actions: EnvironmentActions { + on_enter, + on_wrap_env_enter, + on_wrap_task_run, + on_wrap_env_exit, + on_exit: None, + }, + embedded_files: Some(embedded_files), + }), + variables: None, + resolved_symtab: None, + } +} + +/// Wrap env with `embedded_files` and an `onWrapTaskRun` that references +/// `{{Env.File.config}}`. The wrap env has NO `onEnter` — only the wrap +/// hook. Asserts that the hook resolves successfully and the wrapped +/// command runs (via trace-file content). +#[tokio::test] +async fn wrap_task_run_resolves_env_file_without_on_enter() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + let wrap_script = format!( + r#"echo "[wrap-task] file={{{{Env.File.config}}}}" >> '{}'"#, + trace.display(), + ); + let wrap_task = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&wrap_script)]), + timeout: None, + cancelation: None, + }; + let embedded = openjd_model::job::EmbeddedFile { + name: "config".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("wrap-config.txt".to_string()), + data: Some(fs("wrap-file-data")), + runnable: None, + end_of_line: None, + }; + let env = wrap_env_with_files("Wrapper", None, None, Some(wrap_task), None, vec![embedded]); + session + .enter_environment(&env, None, None, None) + .await + .unwrap(); + + let task_cmd = format!("echo task-ran >> '{}'", trace.display()); + let s = step("sh", vec!["-c", &task_cmd]); + let result = session + .run_task("test_step", &s, None, None, None) + .await + .unwrap(); + assert_eq!(result.state, ActionState::Success); + + let contents = read_trace(&trace); + // The hook resolved Env.File.config to a path + assert!( + contents.contains("[wrap-task] file="), + "wrap hook must resolve Env.File.config; got:\n{contents}" + ); + // The path should point into the embedded_files directory + assert!( + contents.contains("embedded_files/wrap-config.txt"), + "Env.File.config path must end in embedded_files/wrap-config.txt; got:\n{contents}" + ); +} + +/// Wrap env with `embedded_files`, an `onEnter`, AND an `onWrapTaskRun` +/// that references `{{Env.File.config}}`. This is the field-reported +/// combination. Asserts the file resolves and the task runs. +#[tokio::test] +async fn wrap_task_run_resolves_env_file_with_on_enter() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + let wrap_script = format!( + r#"echo "[wrap-task] file={{{{Env.File.config}}}}" >> '{}'"#, + trace.display(), + ); + let wrap_task = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&wrap_script)]), + timeout: None, + cancelation: None, + }; + let embedded = openjd_model::job::EmbeddedFile { + name: "config".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("wrap-config.txt".to_string()), + data: Some(fs("wrap-file-data")), + runnable: None, + end_of_line: None, + }; + let on_enter = action_with_command("true", vec![]); + let env = wrap_env_with_files( + "Wrapper", + Some(on_enter), + None, + Some(wrap_task), + None, + vec![embedded], + ); + session + .enter_environment(&env, None, None, None) + .await + .unwrap(); + + let task_cmd = format!("echo task-ran >> '{}'", trace.display()); + let s = step("sh", vec!["-c", &task_cmd]); + let result = session + .run_task("test_step", &s, None, None, None) + .await + .unwrap(); + assert_eq!(result.state, ActionState::Success); + + let contents = read_trace(&trace); + assert!( + contents.contains("[wrap-task] file="), + "wrap hook must resolve Env.File.config; got:\n{contents}" + ); + assert!( + contents.contains("embedded_files/wrap-config.txt"), + "Env.File.config path must end in embedded_files/wrap-config.txt; got:\n{contents}" + ); +} + +/// `Env.File` referenced in BOTH `onWrapEnvEnter` AND `onWrapEnvExit`. +/// Both hooks must see the resolved path. +#[tokio::test] +async fn env_file_resolves_in_wrap_env_enter_and_exit() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + let enter_script = format!( + r#"echo "[wrap-enter] file={{{{Env.File.cfg}}}}" >> '{}'"#, + trace.display(), + ); + let exit_script = format!( + r#"echo "[wrap-exit] file={{{{Env.File.cfg}}}}" >> '{}'"#, + trace.display(), + ); + let wrap_enter = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&enter_script)]), + timeout: None, + cancelation: None, + }; + let wrap_exit = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&exit_script)]), + timeout: None, + cancelation: None, + }; + let embedded = openjd_model::job::EmbeddedFile { + name: "cfg".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("my-cfg.txt".to_string()), + data: Some(fs("cfg-content")), + runnable: None, + end_of_line: None, + }; + let on_enter = action_with_command("true", vec![]); + let env = wrap_env_with_files( + "Wrapper", + Some(on_enter), + Some(wrap_enter), + None, + Some(wrap_exit), + vec![embedded], + ); + session + .enter_environment(&env, None, None, None) + .await + .unwrap(); + + // Enter an inner env so onWrapEnvEnter fires + let inner = plain_env( + "Inner", + Some(action_with_command("true", vec![])), + Some(action_with_command("true", vec![])), + ); + let inner_id = session + .enter_environment(&inner, None, None, None) + .await + .unwrap(); + // Exit the inner env so onWrapEnvExit fires + session + .exit_environment(&inner_id, None, true, None) + .await + .unwrap(); + + let contents = read_trace(&trace); + assert!( + contents.contains("[wrap-enter] file=") && contents.contains("embedded_files/my-cfg.txt"), + "onWrapEnvEnter must resolve Env.File.cfg; got:\n{contents}" + ); + // Check that the exit hook also resolved the path + let lines: Vec<&str> = contents.lines().collect(); + let exit_line = lines.iter().find(|l| l.contains("[wrap-exit] file=")); + assert!( + exit_line.is_some() && exit_line.unwrap().contains("embedded_files/my-cfg.txt"), + "onWrapEnvExit must resolve Env.File.cfg; got:\n{contents}" + ); +} + +/// PATH STABILITY: running TWO tasks under the same wrap env that +/// references `{{Env.File.config}}` must yield the SAME path both times. +/// This pins the caching design — without it the cache is untested. +#[tokio::test] +async fn env_file_path_stable_across_task_invocations() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + let wrap_script = format!( + r#"echo "PATH={{{{Env.File.config}}}}" >> '{}'"#, + trace.display(), + ); + let wrap_task = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&wrap_script)]), + timeout: None, + cancelation: None, + }; + let embedded = openjd_model::job::EmbeddedFile { + name: "config".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("stable.txt".to_string()), + data: Some(fs("data")), + runnable: None, + end_of_line: None, + }; + let on_enter = action_with_command("true", vec![]); + let env = wrap_env_with_files( + "Wrapper", + Some(on_enter), + None, + Some(wrap_task), + None, + vec![embedded], + ); + session + .enter_environment(&env, None, None, None) + .await + .unwrap(); + + // Run two tasks + let s = step("true", vec![]); + session + .run_task("step1", &s, None, None, None) + .await + .unwrap(); + session + .run_task("step2", &s, None, None, None) + .await + .unwrap(); + + let contents = read_trace(&trace); + let paths: Vec<&str> = contents + .lines() + .filter_map(|l| l.strip_prefix("PATH=")) + .collect(); + assert_eq!( + paths.len(), + 2, + "expected two PATH= lines from two task runs; got:\n{contents}" + ); + assert_eq!( + paths[0], paths[1], + "Env.File path must be stable across invocations (caching); got paths: {:?}", + paths + ); +} + +/// The wrap env's `let` bindings can reference `{{Env.File.*}}`. This +/// tests the register-before-seed ordering: file paths must be +/// registered BEFORE seed_wrapped_action_symbols evaluates the wrap +/// env's let bindings. +#[tokio::test] +async fn wrap_env_let_can_reference_env_file() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + let wrap_script = format!( + r#"echo "[wrap-task] mypath={{{{mypath}}}}" >> '{}'"#, + trace.display(), + ); + let wrap_task = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&wrap_script)]), + timeout: None, + cancelation: None, + }; + let embedded = openjd_model::job::EmbeddedFile { + name: "script".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("run.sh".to_string()), + data: Some(fs("#!/bin/bash\necho hi")), + runnable: None, + end_of_line: None, + }; + let mut env = wrap_env_with_files( + "Wrapper", + Some(action_with_command("true", vec![])), + None, + Some(wrap_task), + None, + vec![embedded], + ); + // The let binding references Env.File.script — this only works if + // file paths are registered BEFORE let evaluation in the seed call. + env.script.as_mut().unwrap().let_bindings = Some(vec!["mypath = Env.File.script".to_string()]); + session + .enter_environment(&env, None, None, None) + .await + .unwrap(); + + let s = step("true", vec![]); + let result = session + .run_task("test_step", &s, None, None, None) + .await + .unwrap(); + assert_eq!(result.state, ActionState::Success); + + let contents = read_trace(&trace); + assert!( + contents.contains("[wrap-task] mypath=") && contents.contains("embedded_files/run.sh"), + "let binding referencing Env.File.script must resolve; got:\n{contents}" + ); +} + +/// PATH STABILITY (unnamed embedded file): unnamed files get a random hex +/// filename at allocation time. Without the cache, every wrap-hook +/// invocation would re-allocate a DIFFERENT random path. This test runs +/// TWO tasks under the same wrap env and asserts the `{{Env.File.scratch}}` +/// path is IDENTICAL both times — only possible if the cache prevents +/// re-allocation. +#[tokio::test] +async fn unnamed_env_file_path_stable_across_task_invocations() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + // Unnamed file allocation writes to the embedded_files dir immediately. + std::fs::create_dir_all(tmp.path().join("embedded_files")).unwrap(); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + let wrap_script = format!( + r#"echo "PATH={{{{Env.File.scratch}}}}" >> '{}'"#, + trace.display(), + ); + let wrap_task = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&wrap_script)]), + timeout: None, + cancelation: None, + }; + // UNNAMED embedded file — filename: None triggers random hex path generation. + let embedded = openjd_model::job::EmbeddedFile { + name: "scratch".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: None, + data: Some(fs("scratch-data")), + runnable: None, + end_of_line: None, + }; + let on_enter = action_with_command("true", vec![]); + let env = wrap_env_with_files( + "Wrapper", + Some(on_enter), + None, + Some(wrap_task), + None, + vec![embedded], + ); + session + .enter_environment(&env, None, None, None) + .await + .unwrap(); + + // Run two tasks under the same wrap env + let s = step("true", vec![]); + session + .run_task("step1", &s, None, None, None) + .await + .unwrap(); + session + .run_task("step2", &s, None, None, None) + .await + .unwrap(); + + let contents = read_trace(&trace); + let paths: Vec<&str> = contents + .lines() + .filter_map(|l| l.strip_prefix("PATH=")) + .collect(); + assert_eq!( + paths.len(), + 2, + "expected two PATH= lines from two task runs; got:\n{contents}" + ); + // Both paths must be non-empty and contain the files directory + assert!( + !paths[0].is_empty() && paths[0].contains("embedded_files"), + "first path must be a valid embedded file path; got: '{}'", + paths[0] + ); + assert_eq!( + paths[0], paths[1], + "unnamed Env.File path must be stable across invocations (caching); \ + different paths means the cache was bypassed and a new random filename \ + was generated per task. paths: {:?}", + paths + ); +} + +/// MULTI-FILE: a wrap env with TWO embedded files (one named, one unnamed) +/// referenced in the same hook. Guards against a register_file_paths +/// implementation that only re-registers the first record. +#[tokio::test] +async fn multiple_embedded_files_all_resolve_in_wrap_hook() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + std::fs::create_dir_all(tmp.path().join("embedded_files")).unwrap(); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + let wrap_script = format!( + r#"echo "NAMED={{{{Env.File.named_cfg}}}}" >> '{path}' +echo "UNNAMED={{{{Env.File.unnamed_scratch}}}}" >> '{path}'"#, + path = trace.display(), + ); + let wrap_task = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&wrap_script)]), + timeout: None, + cancelation: None, + }; + let named_file = openjd_model::job::EmbeddedFile { + name: "named_cfg".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("config.txt".to_string()), + data: Some(fs("named-data")), + runnable: None, + end_of_line: None, + }; + let unnamed_file = openjd_model::job::EmbeddedFile { + name: "unnamed_scratch".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: None, + data: Some(fs("unnamed-data")), + runnable: None, + end_of_line: None, + }; + let on_enter = action_with_command("true", vec![]); + let env = wrap_env_with_files( + "Wrapper", + Some(on_enter), + None, + Some(wrap_task), + None, + vec![named_file, unnamed_file], + ); + session + .enter_environment(&env, None, None, None) + .await + .unwrap(); + + let s = step("true", vec![]); + let result = session + .run_task("test_step", &s, None, None, None) + .await + .unwrap(); + assert_eq!(result.state, ActionState::Success); + + let contents = read_trace(&trace); + // Named file resolves to its explicit filename + let named_line = contents + .lines() + .find(|l| l.starts_with("NAMED=")) + .expect("expected NAMED= line in trace"); + let named_path = named_line.strip_prefix("NAMED=").unwrap(); + assert!( + named_path.contains("embedded_files/config.txt"), + "named file must resolve to embedded_files/config.txt; got: '{named_path}'" + ); + // Unnamed file resolves to a hex-named path in the embedded_files dir + let unnamed_line = contents + .lines() + .find(|l| l.starts_with("UNNAMED=")) + .expect("expected UNNAMED= line in trace"); + let unnamed_path = unnamed_line.strip_prefix("UNNAMED=").unwrap(); + assert!( + unnamed_path.contains("embedded_files/"), + "unnamed file must resolve to a path in embedded_files/; got: '{unnamed_path}'" + ); + // The two paths must be different files + assert_ne!( + named_path, unnamed_path, + "named and unnamed files must have different paths" + ); +} + +/// NOT-REFERENCED: a wrap env with embedded files whose hook does NOT +/// reference `{{Env.File.*}}` at all. Guards that the allocate/register +/// path (which always runs) does not fail or interfere with hooks that +/// never use the file symbols. +#[tokio::test] +async fn wrap_hook_succeeds_when_embedded_files_not_referenced() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + std::fs::create_dir_all(tmp.path().join("embedded_files")).unwrap(); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + // Hook just echoes a literal — no {{Env.File.*}} reference. + let wrap_script = format!(r#"echo "HOOK_RAN=yes" >> '{}'"#, trace.display(),); + let wrap_task = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&wrap_script)]), + timeout: None, + cancelation: None, + }; + let embedded = openjd_model::job::EmbeddedFile { + name: "unused_file".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: None, + data: Some(fs("some-data")), + runnable: None, + end_of_line: None, + }; + let on_enter = action_with_command("true", vec![]); + let env = wrap_env_with_files( + "Wrapper", + Some(on_enter), + None, + Some(wrap_task), + None, + vec![embedded], + ); + session + .enter_environment(&env, None, None, None) + .await + .unwrap(); + + let s = step("true", vec![]); + let result = session + .run_task("test_step", &s, None, None, None) + .await + .unwrap(); + assert_eq!(result.state, ActionState::Success); + + let contents = read_trace(&trace); + assert_eq!( + contents.trim(), + "HOOK_RAN=yes", + "hook must succeed even when embedded files are not referenced; got:\n{contents}" + ); +} + +/// MULTI-FILE PATH STABILITY: two embedded files (named + unnamed) both +/// remain stable across multiple task invocations. Strengthens the +/// single-file stability test by verifying register_file_paths re-registers +/// ALL cached records, not just the first. +#[tokio::test] +async fn multiple_embedded_files_paths_stable_across_tasks() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + std::fs::create_dir_all(tmp.path().join("embedded_files")).unwrap(); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + let wrap_script = format!( + r#"echo "N={{{{Env.File.named_f}}}}" >> '{path}' +echo "U={{{{Env.File.unnamed_f}}}}" >> '{path}'"#, + path = trace.display(), + ); + let wrap_task = Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&wrap_script)]), + timeout: None, + cancelation: None, + }; + let named_file = openjd_model::job::EmbeddedFile { + name: "named_f".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("stable-named.txt".to_string()), + data: Some(fs("named-content")), + runnable: None, + end_of_line: None, + }; + let unnamed_file = openjd_model::job::EmbeddedFile { + name: "unnamed_f".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: None, + data: Some(fs("unnamed-content")), + runnable: None, + end_of_line: None, + }; + let on_enter = action_with_command("true", vec![]); + let env = wrap_env_with_files( + "Wrapper", + Some(on_enter), + None, + Some(wrap_task), + None, + vec![named_file, unnamed_file], + ); + session + .enter_environment(&env, None, None, None) + .await + .unwrap(); + + let s = step("true", vec![]); + session + .run_task("step1", &s, None, None, None) + .await + .unwrap(); + session + .run_task("step2", &s, None, None, None) + .await + .unwrap(); + + let contents = read_trace(&trace); + let named_paths: Vec<&str> = contents + .lines() + .filter_map(|l| l.strip_prefix("N=")) + .collect(); + let unnamed_paths: Vec<&str> = contents + .lines() + .filter_map(|l| l.strip_prefix("U=")) + .collect(); + assert_eq!( + named_paths.len(), + 2, + "expected two N= lines; got:\n{contents}" + ); + assert_eq!( + unnamed_paths.len(), + 2, + "expected two U= lines; got:\n{contents}" + ); + assert_eq!( + named_paths[0], named_paths[1], + "named file path must be stable across tasks; got: {:?}", + named_paths + ); + assert_eq!( + unnamed_paths[0], unnamed_paths[1], + "unnamed file path must be stable across tasks (caching); \ + different paths means re-allocation occurred. got: {:?}", + unnamed_paths + ); +} diff --git a/specs/sessions/embedded-files.md b/specs/sessions/embedded-files.md index 3fedf93f..2205012f 100644 --- a/specs/sessions/embedded-files.md +++ b/specs/sessions/embedded-files.md @@ -163,3 +163,54 @@ This is what makes `Env.File.*` / `Task.File.*` available to `let` bindings while letting file `data` reference let-bound values. It is only possible because `filename` is a plain string (2023-09 schema, not `@fmtstring`), so path allocation never depends on `let` values. + +## Wrap Environment File Path Caching + +Wrap hooks (`onWrapEnvEnter`, `onWrapTaskRun`, `onWrapEnvExit`) may reference +the wrap environment's `Env.File.*` symbols. Unlike normal environment/step +scripts where each action invocation has a fresh `EmbeddedFiles` instance, wrap +hooks fire repeatedly (once per inner environment enter/exit, once per task) and +must present stable file paths across invocations. + +### Design + +The `Session` maintains a per-wrap-environment cache +(`wrap_env_file_records: HashMap`): + +- **First invocation (cache miss):** `allocate_file_paths()` allocates paths + and writes empty files (unnamed) or validates filenames (named). The + `EmbeddedFiles` instance is stored in the cache. +- **Subsequent invocations (cache hit):** `register_file_paths()` re-registers + the previously allocated paths into the current action's symbol table without + allocating new paths or creating files on disk. +- **Every invocation:** `write_file_contents()` is called AFTER + `seed_wrapped_action_symbols()` so file `data` resolves against the post-lets + symbol table. This means each invocation starts from the authored content, + which may reference values that change between invocations. + +### Ordering + +The ordering at each wrap-hook dispatch site is: + +``` +register/allocate file paths → seed_wrapped_action_symbols (evaluates lets) → write file contents +``` + +This matches the standard two-phase flow (`allocate → lets → write`) and +ensures the wrap env's `let` bindings can reference `{{Env.File.*}}`. + +### Eviction + +The cache entry is removed when the wrap environment itself is exited +(`exit_environment`). The on-disk files are NOT deleted — they reside in the +session's files directory and are cleaned up with it at session end. + +### `register_file_paths` + +```rust +pub(crate) fn register_file_paths(&self, symtab: &mut SymbolTable) -> Result<(), SessionError>; +``` + +Iterates the previously allocated `FileRecord`s and sets each record's symbol +to its filename path in the symbol table. Does not allocate paths, create files, +or mutate `self.records`. Logs a "Reusing embedded file paths" message. diff --git a/specs/sessions/session.md b/specs/sessions/session.md index 3ac8afb8..0364af9a 100644 --- a/specs/sessions/session.md +++ b/specs/sessions/session.md @@ -173,8 +173,10 @@ in an inconsistent state. The Python library enforces this, and the Rust crate m When a wrap hook is dispatched, the borrow of the active environment stack is released by copying only the wrapper data needed for symbol seeding: its name, frozen resolved symbol table, script `let` bindings, and selected hook action. -The wrapper's embedded files and other environment fields are not cloned per -task. +The wrapper's embedded files are handled separately via a per-wrap-environment +cache (see [embedded-files.md](embedded-files.md#wrap-environment-file-path-caching)): +file paths are allocated once and reused across invocations; file contents are +re-written each time so `data` expressions resolve against the current scope. ## Ad-hoc Subprocess From 6ff385b114cc361418dcfc31bbb07b72c957ce27 Mon Sep 17 00:00:00 2001 From: Sean Tang <171081544+seant-aws@users.noreply.github.com> Date: Thu, 13 Aug 2026 19:01:32 +0000 Subject: [PATCH 2/2] fix(sessions): address review feedback on wrap-env embedded files - Move wrap_env_file_records eviction before exit script so the cache is cleared unconditionally (even when onExit fails) - Add isolation tests: same-name shadowing + cross-reference rejection - Update embedded-files.md spec to document error-path eviction Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com> --- crates/openjd-sessions/src/session.rs | 15 +- .../tests/integration/test_wrap_actions.rs | 164 ++++++++++++++++++ specs/sessions/embedded-files.md | 7 +- 3 files changed, 177 insertions(+), 9 deletions(-) diff --git a/crates/openjd-sessions/src/session.rs b/crates/openjd-sessions/src/session.rs index decde4ae..26e7d074 100644 --- a/crates/openjd-sessions/src/session.rs +++ b/crates/openjd-sessions/src/session.rs @@ -1425,6 +1425,14 @@ impl Session { })?; self.environments_entered.pop(); + // Evict the wrap-env file cache unconditionally alongside other + // environment teardown. This runs before the exit script so the + // cache is cleared even when the exit script fails — a re-entered + // environment always gets fresh allocations. + if env_has_any_wrap_hook(&env) { + self.wrap_env_file_records.remove(identifier); + } + let output = if env .script .as_ref() @@ -1629,13 +1637,6 @@ impl Session { String::new() }; - // Evict the wrap-env file cache when the wrap environment itself is - // exited. The on-disk files are intentionally NOT deleted — they live - // in the session files directory and are cleaned up with it. - if env_has_any_wrap_hook(&env) { - self.wrap_env_file_records.remove(identifier); - } - Ok(output) } diff --git a/crates/openjd-sessions/tests/integration/test_wrap_actions.rs b/crates/openjd-sessions/tests/integration/test_wrap_actions.rs index 5a8ea3fa..f36bf243 100644 --- a/crates/openjd-sessions/tests/integration/test_wrap_actions.rs +++ b/crates/openjd-sessions/tests/integration/test_wrap_actions.rs @@ -1858,3 +1858,167 @@ echo "U={{{{Env.File.unnamed_f}}}}" >> '{path}'"#, unnamed_paths ); } + +/// ISOLATION: when a wrap env and an inner env both declare an embedded file +/// named `config`, the inner env's `onEnter` sees its OWN file (the inner's +/// `Env.File.config` shadows the wrapper's). This proves that the wrap env's +/// cached `Env.File.*` entries do not leak into the inner environment's +/// standard execution scope. +#[tokio::test] +async fn wrap_env_file_same_name_inner_shadows_correctly() { + let tmp = TempDir::new().unwrap(); + let trace = tmp.path().join("trace.log"); + std::fs::create_dir_all(tmp.path().join("embedded_files")).unwrap(); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + // Wrap env declares an embedded file named "config" with wrap-specific content. + // It defines onWrapTaskRun (making it a wrap env) but NOT onWrapEnvEnter, + // so the inner env's onEnter runs directly through the standard runner path. + let wrap_task = action_with_command("true", vec![]); + let wrap_embedded = openjd_model::job::EmbeddedFile { + name: "config".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("wrap-config.txt".to_string()), + data: Some(fs("wrap-content")), + runnable: None, + end_of_line: None, + }; + let outer = wrap_env_with_files( + "Wrapper", + Some(action_with_command("true", vec![])), + None, + Some(wrap_task), + None, + vec![wrap_embedded], + ); + session + .enter_environment(&outer, None, None, None) + .await + .unwrap(); + + // Inner env ALSO declares an embedded file named "config" — same symbol + // name, different content. Its onEnter cats the file to prove which one + // it sees. + let inner_enter_cmd = format!( + "cat \"{{{{Env.File.config}}}}\" >> '{}'", + trace.display() + ); + let inner_embedded = openjd_model::job::EmbeddedFile { + name: "config".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("inner-config.txt".to_string()), + data: Some(fs("inner-content")), + runnable: None, + end_of_line: None, + }; + let inner = Environment { + name: "Inner".to_string(), + description: None, + script: Some(EnvironmentScript { + let_bindings: None, + actions: EnvironmentActions { + on_enter: Some(Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs(&inner_enter_cmd)]), + timeout: None, + cancelation: None, + }), + on_wrap_env_enter: None, + on_wrap_task_run: None, + on_wrap_env_exit: None, + on_exit: None, + }, + embedded_files: Some(vec![inner_embedded]), + }), + variables: None, + resolved_symtab: None, + }; + session + .enter_environment(&inner, None, None, None) + .await + .unwrap(); + + let contents = read_trace(&trace); + assert!( + contents.contains("inner-content"), + "inner env's onEnter must see its OWN Env.File.config, not the wrapper's; got:\n{contents}" + ); + assert!( + !contents.contains("wrap-content"), + "wrapper's Env.File.config must NOT leak into the inner env's scope; got:\n{contents}" + ); +} + +/// ISOLATION: when a wrap env declares an embedded file named `wrapper_only`, +/// an inner environment that does NOT declare that file cannot reference +/// `{{Env.File.wrapper_only}}` — the symbol is undefined in the inner's scope. +/// This proves that the wrap env's `Env.File.*` entries are scoped to the wrap +/// hooks only and do not pollute inner environments' execution. +#[tokio::test] +async fn wrap_env_file_different_name_inner_cannot_reference_wrapper_file() { + let tmp = TempDir::new().unwrap(); + std::fs::create_dir_all(tmp.path().join("embedded_files")).unwrap(); + let mut session = Session::new_for_test(tmp.path().to_path_buf()); + + // Wrap env declares an embedded file named "wrapper_only". + let wrap_task = action_with_command("true", vec![]); + let wrap_embedded = openjd_model::job::EmbeddedFile { + name: "wrapper_only".to_string(), + file_type: openjd_model::types::FileType::Text, + filename: Some("secret.txt".to_string()), + data: Some(fs("secret")), + runnable: None, + end_of_line: None, + }; + let outer = wrap_env_with_files( + "Wrapper", + Some(action_with_command("true", vec![])), + None, + Some(wrap_task), + None, + vec![wrap_embedded], + ); + session + .enter_environment(&outer, None, None, None) + .await + .unwrap(); + + // Inner env does NOT declare any embedded files but its onEnter attempts + // to reference {{Env.File.wrapper_only}}. This must fail because the + // symbol is not defined in the inner env's scope. + let inner = Environment { + name: "Inner".to_string(), + description: None, + script: Some(EnvironmentScript { + let_bindings: None, + actions: EnvironmentActions { + on_enter: Some(Action { + command: fs("bash"), + args: Some(vec![fs("-c"), fs("cat {{Env.File.wrapper_only}}")]), + timeout: None, + cancelation: None, + }), + on_wrap_env_enter: None, + on_wrap_task_run: None, + on_wrap_env_exit: None, + on_exit: None, + }, + embedded_files: None, + }), + variables: None, + resolved_symtab: None, + }; + let result = session + .enter_environment(&inner, None, None, None) + .await; + assert!( + result.is_err(), + "referencing Env.File.wrapper_only from the inner env must fail; \ + the wrap env's embedded files must not leak into inner scope" + ); + let err_msg = result.unwrap_err().to_string(); + assert!( + err_msg.contains("Env.File.wrapper_only"), + "error must name the undefined symbol; got: {err_msg}" + ); +} diff --git a/specs/sessions/embedded-files.md b/specs/sessions/embedded-files.md index 2205012f..4cc71b72 100644 --- a/specs/sessions/embedded-files.md +++ b/specs/sessions/embedded-files.md @@ -201,8 +201,11 @@ ensures the wrap env's `let` bindings can reference `{{Env.File.*}}`. ### Eviction -The cache entry is removed when the wrap environment itself is exited -(`exit_environment`). The on-disk files are NOT deleted — they reside in the +The cache entry is removed unconditionally alongside other environment teardown +(the `environments.remove` and `environments_entered.pop` calls) in +`exit_environment`, before the exit script runs. This ensures the cache is +cleared even when the exit script fails, so a re-entered environment always gets +fresh allocations. The on-disk files are NOT deleted — they reside in the session's files directory and are cleaned up with it at session end. ### `register_file_paths`