mirror of
https://github.com/bahdotsh/wrkflw.git
synced 2026-09-01 19:50:23 +02:00
fix(executor): close silent failure modes found in PR review
Four things came out of the review that were all variations of the
same theme: "something goes wrong and nobody hears about it."
The cache restore task was swallowing panics via `.ok()?`, which is
a lovely way to make debugging impossible. Match the error handling
pattern already used in save() — log the panic, return None.
The GITHUB_OUTPUT write for cache-hit was using `let _ =` to discard
I/O errors. This was the *only* silent error discard in the entire
module, which makes it feel less like a deliberate choice and more
like an oversight. Log the failure instead.
`secrets: inherit` on reusable workflows was silently ignored because
the code only handled the mapping case and inherit comes through as
a bare string. Emit an explicit warning so users know their secrets
aren't actually being inherited yet.
While at it, replace `&line[2..]` with `strip_prefix("::")` in the
workflow command parser. The starts_with check above guarantees
safety, but direct byte indexing is fragile and strip_prefix says
what it means.
This commit is contained in:
@@ -65,11 +65,17 @@ impl CacheStore {
|
||||
let path = path.to_string();
|
||||
let workspace = workspace.to_path_buf();
|
||||
|
||||
tokio::task::spawn_blocking(move || {
|
||||
match tokio::task::spawn_blocking(move || {
|
||||
this.restore_inner(&key, &restore_keys, &path, &workspace)
|
||||
})
|
||||
.await
|
||||
.ok()?
|
||||
{
|
||||
Ok(result) => result,
|
||||
Err(e) => {
|
||||
wrkflw_logging::warning(&format!("Cache restore task panicked: {}", e));
|
||||
None
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Save the contents of `path` (relative to `workspace`) under `key`.
|
||||
|
||||
@@ -2853,13 +2853,19 @@ async fn execute_step(ctx: StepExecutionContext<'_>) -> Result<StepResult, Execu
|
||||
// Write cache-hit output to GITHUB_OUTPUT file
|
||||
if let Some(output_path) = ctx.job_env.get("GITHUB_OUTPUT") {
|
||||
let hit_val = if cache_hit.is_some() { "true" } else { "false" };
|
||||
let _ = std::fs::OpenOptions::new()
|
||||
if let Err(e) = std::fs::OpenOptions::new()
|
||||
.append(true)
|
||||
.open(output_path)
|
||||
.and_then(|mut f| {
|
||||
use std::io::Write;
|
||||
writeln!(f, "cache-hit={}", hit_val)
|
||||
});
|
||||
})
|
||||
{
|
||||
wrkflw_logging::warning(&format!(
|
||||
"Failed to write cache-hit to GITHUB_OUTPUT: {}",
|
||||
e
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
match &cache_hit {
|
||||
@@ -4118,7 +4124,12 @@ async fn execute_reusable_workflow_job(
|
||||
}
|
||||
}
|
||||
if let Some(secrets_val) = secrets {
|
||||
if let Some(map) = secrets_val.as_mapping() {
|
||||
if secrets_val.as_str() == Some("inherit") {
|
||||
wrkflw_logging::warning(
|
||||
"`secrets: inherit` is not yet supported for reusable workflows; \
|
||||
parent secrets will not be available in the called workflow",
|
||||
);
|
||||
} else if let Some(map) = secrets_val.as_mapping() {
|
||||
for (k, v) in map {
|
||||
if let (Some(key), Some(value)) = (k.as_str(), v.as_str()) {
|
||||
child_env.insert(
|
||||
@@ -4131,8 +4142,6 @@ async fn execute_reusable_workflow_job(
|
||||
}
|
||||
|
||||
// Execute called workflow, reusing parent's artifact/cache stores.
|
||||
// TODO: propagate parent secret_manager/secret_masker when `secrets: inherit`
|
||||
// is specified (currently reusable workflow jobs cannot access parent secrets).
|
||||
let plan = dependency::resolve_dependencies(&called)?;
|
||||
let mut all_results = Vec::new();
|
||||
let mut any_failed = false;
|
||||
@@ -4210,7 +4219,12 @@ async fn execute_reusable_workflow_job(
|
||||
}
|
||||
}
|
||||
if let Some(secrets_val) = secrets {
|
||||
if let Some(map) = secrets_val.as_mapping() {
|
||||
if secrets_val.as_str() == Some("inherit") {
|
||||
wrkflw_logging::warning(
|
||||
"`secrets: inherit` is not yet supported for reusable workflows; \
|
||||
parent secrets will not be available in the called workflow",
|
||||
);
|
||||
} else if let Some(map) = secrets_val.as_mapping() {
|
||||
for (k, v) in map {
|
||||
if let (Some(key), Some(value)) = (k.as_str(), v.as_str()) {
|
||||
child_env.insert(format!("SECRET_{}", key.to_uppercase()), value.to_string());
|
||||
@@ -4220,8 +4234,6 @@ async fn execute_reusable_workflow_job(
|
||||
}
|
||||
|
||||
// Execute called workflow, reusing parent's artifact/cache stores.
|
||||
// TODO: propagate parent secret_manager/secret_masker when `secrets: inherit`
|
||||
// is specified (currently reusable workflow jobs cannot access parent secrets).
|
||||
let plan = dependency::resolve_dependencies(&called)?;
|
||||
let mut all_results = Vec::new();
|
||||
let mut any_failed = false;
|
||||
|
||||
@@ -90,7 +90,7 @@ pub fn parse_workflow_commands(output: &str) -> Vec<WorkflowCommand> {
|
||||
fn parse_command_line(line: &str) -> Option<WorkflowCommand> {
|
||||
// Format: ::command param1=val1,param2=val2::message
|
||||
// The line starts with "::" — strip it.
|
||||
let rest = &line[2..];
|
||||
let rest = line.strip_prefix("::").unwrap_or(line);
|
||||
|
||||
// Find the second "::" that separates command+params from the message
|
||||
let (cmd_part, message) = if let Some(idx) = rest.find("::") {
|
||||
|
||||
Reference in New Issue
Block a user