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..26e7d074 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 @@ -1397,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() @@ -1440,6 +1476,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 +1513,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 @@ -1632,15 +1688,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 +1758,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 +2599,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..f36bf243 100644 --- a/crates/openjd-sessions/tests/integration/test_wrap_actions.rs +++ b/crates/openjd-sessions/tests/integration/test_wrap_actions.rs @@ -1185,3 +1185,840 @@ 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 + ); +} + +/// 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 3fedf93f..4cc71b72 100644 --- a/specs/sessions/embedded-files.md +++ b/specs/sessions/embedded-files.md @@ -163,3 +163,57 @@ 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 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` + +```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