From cd3789e4c5eb05c5d5dd9506cb79cdcd8568358a Mon Sep 17 00:00:00 2001
From: bahdotsh <appu.yess@gmail.com>
Date: Sun, 5 Apr 2026 20:44:23 +0530
Subject: [PATCH] fix(executor): close silent failure modes found in PR review
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 crates/executor/src/cache.rs             | 10 +++++++--
 crates/executor/src/engine.rs            | 28 +++++++++++++++++-------
 crates/executor/src/workflow_commands.rs |  2 +-
 3 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/crates/executor/src/cache.rs b/crates/executor/src/cache.rs
index 0ab5488..a91d7c7 100644
--- a/crates/executor/src/cache.rs
+++ b/crates/executor/src/cache.rs
@@ -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`.
diff --git a/crates/executor/src/engine.rs b/crates/executor/src/engine.rs
index 5c12de6..f2cc031 100644
--- a/crates/executor/src/engine.rs
+++ b/crates/executor/src/engine.rs
@@ -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;
diff --git a/crates/executor/src/workflow_commands.rs b/crates/executor/src/workflow_commands.rs
index 903a01e..7371131 100644
--- a/crates/executor/src/workflow_commands.rs
+++ b/crates/executor/src/workflow_commands.rs
@@ -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("::") {