Mirrors/wrkflw
1
0
Fork 0
mirror of https://github.com/bahdotsh/wrkflw.git synced 2026-07-09 20:09:35 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(executor): fix env file read-back bugs and deduplicate post-step logic

Browse Source 
The previous commit added environment file read-back but had a few
issues that would bite in real workflows.

First, the heredoc parser was checking for `<<` before checking for
`=`, which means a value like `url=https://example.com/path<<EOF`
would be misinterpreted as a heredoc start. The fix is obvious: the
key before `<<` must actually be a valid identifier, not just "any
non-empty string". Add is_valid_identifier() to enforce that.

Second, GITHUB_ENV and GITHUB_PATH files were never truncated between
steps. Since we read and merge their *entire* contents after each
step, step 2 would re-process step 1's entries, causing duplicate
PATH entries to accumulate O(n²) with each step. We already merge
into job_env which is the source of truth — just truncate all three
files after read-back.

Third, the ~25 lines of post-step read-back logic were copy-pasted
between execute_job and execute_matrix_job. Extract into
apply_step_environment_updates() so there's exactly one place to
maintain this.

While at it, add tests for the heredoc ambiguity, unterminated
heredocs, the apply helper, and — most importantly — a multi-step
test that verifies PATH entries don't duplicate across steps.
This commit is contained in:
bahdotsh
2026-04-02 18:57:45 +05:30
parent 4ad1a92b29
commit 57921545da
2 changed files with 218 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

62
crates/executor/src/engine.rs
View File

@@ -1800,33 +1800,12 @@ async fn execute_job(ctx: JobExecutionContext<'_>) -> Result<JobResult, Executio
step_results.push(result);
// Post-step: read back environment files written by the step
let updates = crate::github_env_files::read_step_environment_updates(&job_env);
// Store step outputs keyed by step ID for ${{ steps.<id>.outputs.<key> }}
if let Some(ref step_id) = step.id {
step_outputs_map.insert(step_id.clone(), updates.outputs);
}
// Merge GITHUB_ENV entries into job_env for subsequent steps
for (k, v) in updates.env_vars {
job_env.insert(k, v);
}
// Prepend GITHUB_PATH entries to PATH for subsequent steps
if !updates.path_entries.is_empty() {
let current_path = job_env.get("PATH").cloned().unwrap_or_default();
let new_entries = updates.path_entries.join(":");
let new_path = if current_path.is_empty() {
new_entries
} else {
format!("{}:{}", new_entries, current_path)
};
job_env.insert("PATH".to_string(), new_path);
}
// Clear GITHUB_OUTPUT for next step (per-step, not cumulative)
crate::github_env_files::clear_github_output(&job_env);
// Read back environment files and apply to job state
crate::github_env_files::apply_step_environment_updates(
&mut job_env,
&mut step_outputs_map,
step.id.as_deref(),
);
if abort_job {
job_success = false;
@@ -2030,29 +2009,12 @@ async fn execute_matrix_job(
step_results.push(result);
// Post-step: read back environment files written by the step
let updates = crate::github_env_files::read_step_environment_updates(&job_env);
if let Some(ref step_id) = step.id {
step_outputs_map.insert(step_id.clone(), updates.outputs);
}
for (k, v) in updates.env_vars {
job_env.insert(k, v);
}
if !updates.path_entries.is_empty() {
let current_path = job_env.get("PATH").cloned().unwrap_or_default();
let new_entries = updates.path_entries.join(":");
let new_path = if current_path.is_empty() {
new_entries
} else {
format!("{}:{}", new_entries, current_path)
};
job_env.insert("PATH".to_string(), new_path);
}
crate::github_env_files::clear_github_output(&job_env);
// Read back environment files and apply to job state
crate::github_env_files::apply_step_environment_updates(
&mut job_env,
&mut step_outputs_map,
step.id.as_deref(),
);
if abort_job {
all_steps_ok = false;

216
crates/executor/src/github_env_files.rs
View File

@@ -15,6 +15,16 @@ pub struct StepEnvironmentUpdates {
pub step_summary: String,
}
/// Check whether `s` looks like a valid GHA identifier: `[a-zA-Z_][a-zA-Z0-9_]*`.
fn is_valid_identifier(s: &str) -> bool {
let mut chars = s.chars();
match chars.next() {
Some(c) if c.is_ascii_alphabetic() || c == '_' => {}
_ => return false,
}
chars.all(|c| c.is_ascii_alphanumeric() || c == '_')
}
/// Parse the GitHub Actions key-value file format used by GITHUB_OUTPUT and GITHUB_ENV.
///
/// Supports two formats:
@@ -35,11 +45,13 @@ pub fn parse_github_kv_file(content: &str) -> HashMap<String, String> {
}
// Check for heredoc format: key<<DELIMITER
// The key must be a valid identifier (no '=' allowed) to avoid ambiguity
// with simple values that contain '<<'.
if let Some(heredoc_sep_pos) = line.find("<<") {
let key = &line[..heredoc_sep_pos];
let delimiter = &line[heredoc_sep_pos + 2..];
if !key.is_empty() && !delimiter.is_empty() {
if !key.is_empty() && !delimiter.is_empty() && is_valid_identifier(key) {
// Collect lines until we find the delimiter
let mut value_lines = Vec::new();
i += 1;
@@ -121,13 +133,56 @@ pub fn read_step_environment_updates(job_env: &HashMap<String, String>) -> StepE
updates
}
/// Truncate the GITHUB_OUTPUT file between steps.
/// Apply environment updates from a completed step to the job state.
///
/// Step outputs are per-step (not cumulative), so we clear the file before each step.
/// GITHUB_ENV and GITHUB_PATH are cumulative and should NOT be cleared.
pub fn clear_github_output(job_env: &HashMap<String, String>) {
if let Some(path) = job_env.get("GITHUB_OUTPUT") {
let _ = fs::write(Path::new(path), "");
/// - Stores step outputs keyed by step ID (for `${{ steps.<id>.outputs.<key> }}`)
/// - Merges GITHUB_ENV entries into `job_env`
/// - Prepends GITHUB_PATH entries to the PATH in `job_env`
/// - Clears per-step files so the next step starts fresh
pub fn apply_step_environment_updates(
job_env: &mut HashMap<String, String>,
step_outputs_map: &mut HashMap<String, HashMap<String, String>>,
step_id: Option<&str>,
) {
let updates = read_step_environment_updates(job_env);
// Store step outputs keyed by step ID for ${{ steps.<id>.outputs.<key> }}
if let Some(id) = step_id {
step_outputs_map.insert(id.to_string(), updates.outputs);
}
// Merge GITHUB_ENV entries into job_env for subsequent steps
for (k, v) in updates.env_vars {
job_env.insert(k, v);
}
// Prepend GITHUB_PATH entries to PATH for subsequent steps
if !updates.path_entries.is_empty() {
let current_path = job_env.get("PATH").cloned().unwrap_or_default();
let new_entries = updates.path_entries.join(":");
let new_path = if current_path.is_empty() {
new_entries
} else {
format!("{}:{}", new_entries, current_path)
};
job_env.insert("PATH".to_string(), new_path);
}
// Clear files so the next step doesn't re-process these entries
clear_step_files(job_env);
}
/// Truncate environment files between steps.
///
/// GITHUB_OUTPUT is per-step (not cumulative).
/// GITHUB_ENV and GITHUB_PATH are cumulative *on disk* in real GHA, but we read back
/// and merge their contents into `job_env` after each step. To avoid re-processing
/// the same entries on the next step, we truncate them here as well.
pub fn clear_step_files(job_env: &HashMap<String, String>) {
for key in &["GITHUB_OUTPUT", "GITHUB_ENV", "GITHUB_PATH"] {
if let Some(path) = job_env.get(*key) {
let _ = fs::write(Path::new(path), "");
}
}
}
@@ -227,20 +282,35 @@ mod tests {
fn read_and_clear_round_trip() {
let dir = tempdir().unwrap();
let output_path = dir.path().join("output");
let env_path = dir.path().join("env");
let path_path = dir.path().join("path");
fs::write(&output_path, "version=1.2.3\n").unwrap();
fs::write(&env_path, "MY_VAR=hello\n").unwrap();
fs::write(&path_path, "/new/bin\n").unwrap();
let mut env = HashMap::new();
env.insert(
"GITHUB_OUTPUT".to_string(),
output_path.to_string_lossy().to_string(),
);
env.insert(
"GITHUB_ENV".to_string(),
env_path.to_string_lossy().to_string(),
);
env.insert(
"GITHUB_PATH".to_string(),
path_path.to_string_lossy().to_string(),
);
let updates = read_step_environment_updates(&env);
assert_eq!(updates.outputs.get("version").unwrap(), "1.2.3");
assert_eq!(updates.env_vars.get("MY_VAR").unwrap(), "hello");
assert_eq!(updates.path_entries, vec!["/new/bin"]);
clear_github_output(&env);
let content = fs::read_to_string(&output_path).unwrap();
assert!(content.is_empty());
clear_step_files(&env);
assert!(fs::read_to_string(&output_path).unwrap().is_empty());
assert!(fs::read_to_string(&env_path).unwrap().is_empty());
assert!(fs::read_to_string(&path_path).unwrap().is_empty());
}
#[test]
@@ -281,4 +351,130 @@ mod tests {
assert_eq!(updates.path_entries, vec!["/new/path"]);
assert_eq!(updates.step_summary, "## Summary\nAll good");
}
#[test]
fn parse_value_containing_heredoc_marker() {
// A value like `url=https://example.com/path<<EOF` should be parsed as simple
// key=value, NOT as a heredoc, because the text before `<<` contains `=` and
// is therefore not a valid identifier.
let content = "url=https://example.com/path<<EOF";
let result = parse_github_kv_file(content);
assert_eq!(result.get("url").unwrap(), "https://example.com/path<<EOF");
}
#[test]
fn parse_unterminated_heredoc() {
// Unterminated heredoc should consume to EOF and produce the collected lines.
let content = "body<<EOF\nline1\nline2";
let result = parse_github_kv_file(content);
assert_eq!(result.get("body").unwrap(), "line1\nline2");
}
#[test]
fn parse_heredoc_in_output_format() {
// GITHUB_OUTPUT can use heredoc format for multiline values.
let content = "json<<EOF\n{\"key\": \"value\"}\nEOF\nversion=1.0";
let result = parse_github_kv_file(content);
assert_eq!(result.get("json").unwrap(), "{\"key\": \"value\"}");
assert_eq!(result.get("version").unwrap(), "1.0");
}
#[test]
fn apply_updates_merges_env_and_path() {
let dir = tempdir().unwrap();
let github_dir = dir.path().join("github");
fs::create_dir_all(&github_dir).unwrap();
fs::write(github_dir.join("output"), "artifact=build.tar\n").unwrap();
fs::write(github_dir.join("env"), "CC=gcc\n").unwrap();
fs::write(github_dir.join("path"), "/opt/gcc/bin\n").unwrap();
let mut job_env = HashMap::new();
job_env.insert(
"GITHUB_OUTPUT".to_string(),
github_dir.join("output").to_string_lossy().to_string(),
);
job_env.insert(
"GITHUB_ENV".to_string(),
github_dir.join("env").to_string_lossy().to_string(),
);
job_env.insert(
"GITHUB_PATH".to_string(),
github_dir.join("path").to_string_lossy().to_string(),
);
job_env.insert("PATH".to_string(), "/usr/bin".to_string());
let mut step_outputs_map = HashMap::new();
apply_step_environment_updates(&mut job_env, &mut step_outputs_map, Some("build"));
// Step outputs stored under step ID
assert_eq!(
step_outputs_map
.get("build")
.unwrap()
.get("artifact")
.unwrap(),
"build.tar"
);
// Env merged
assert_eq!(job_env.get("CC").unwrap(), "gcc");
// Path prepended
assert_eq!(job_env.get("PATH").unwrap(), "/opt/gcc/bin:/usr/bin");
// Files cleared for next step
assert!(fs::read_to_string(github_dir.join("output"))
.unwrap()
.is_empty());
assert!(fs::read_to_string(github_dir.join("env"))
.unwrap()
.is_empty());
assert!(fs::read_to_string(github_dir.join("path"))
.unwrap()
.is_empty());
}
#[test]
fn apply_updates_no_duplicate_path_entries() {
let dir = tempdir().unwrap();
let github_dir = dir.path().join("github");
fs::create_dir_all(&github_dir).unwrap();
let output_path = github_dir.join("output");
let env_path = github_dir.join("env");
let path_path = github_dir.join("path");
let mut job_env = HashMap::new();
job_env.insert(
"GITHUB_OUTPUT".to_string(),
output_path.to_string_lossy().to_string(),
);
job_env.insert(
"GITHUB_ENV".to_string(),
env_path.to_string_lossy().to_string(),
);
job_env.insert(
"GITHUB_PATH".to_string(),
path_path.to_string_lossy().to_string(),
);
job_env.insert("PATH".to_string(), "/usr/bin".to_string());
let mut step_outputs_map = HashMap::new();
// Step 1 writes /opt/tool to GITHUB_PATH
fs::write(&output_path, "").unwrap();
fs::write(&env_path, "").unwrap();
fs::write(&path_path, "/opt/tool\n").unwrap();
apply_step_environment_updates(&mut job_env, &mut step_outputs_map, None);
assert_eq!(job_env.get("PATH").unwrap(), "/opt/tool:/usr/bin");
// Step 2 writes /opt/other to GITHUB_PATH
fs::write(&output_path, "").unwrap();
fs::write(&env_path, "").unwrap();
fs::write(&path_path, "/opt/other\n").unwrap();
apply_step_environment_updates(&mut job_env, &mut step_outputs_map, None);
// /opt/tool should appear exactly once (not duplicated)
let path = job_env.get("PATH").unwrap();
assert_eq!(path, "/opt/other:/opt/tool:/usr/bin");
}
}