From 74e8afe9a8f72b1ff0a1f5fd62d78f2eb15f7e15 Mon Sep 17 00:00:00 2001
From: Isaac Hales <ihales@users.noreply.github.com>
Date: Fri, 5 Sep 2025 08:57:58 -0600
Subject: [PATCH] Fix logic for default values for task variables (#37588)

This is a small fix for default values in task variables. The
[documentation](https://zed.dev/docs/tasks) states

> You can also use verbose syntax that allows specifying a default if a
given variable is not available: ${ZED_FILE:default_value}

I found, however, that this doesn't actually work. Instead, the Zed
variable and the default value are just appended in the output. For
example, if I run a task `echo ${ZED_ROW:100}` the result I get is
`447:100` (in this case it should just be `447`).

This PR fixes that. I also added a new test case for handling default
values.
I also tested the fix in a dev build and it seems to work.

There are no UI adjustments.

AI disclosure: I used Claude Code to write the code, including the fix
and the tests.

This is actually my first open-source PR ever, so if I did something
wrong, I'd appreciate any tips and I'll make it right!


Release Notes:

- Fixed task variable substitution always appending the default
---
 crates/task/src/task_template.rs | 92 +++++++++++++++++++++++++++++---
 1 file changed, 85 insertions(+), 7 deletions(-)

diff --git a/crates/task/src/task_template.rs b/crates/task/src/task_template.rs
index 3d1d180557fc457e4200a5b246f2a08e2f5dfcf0..a57f5a175af3fd79ce6b8ef818e3fb97acdc32c2 100644
--- a/crates/task/src/task_template.rs
+++ b/crates/task/src/task_template.rs
@@ -333,15 +333,16 @@ fn substitute_all_template_variables_in_str<A: AsRef<str>>(
             if let Some(substituted_variable) = variable_names.get(variable_name) {
                 substituted_variables.insert(substituted_variable.clone());
             }
-
-            let mut name = name.as_ref().to_owned();
-            // Got a task variable hit
+            // Got a task variable hit - use the variable value, ignore default
+            return Ok(Some(name.as_ref().to_owned()));
+        } else if variable_name.starts_with(ZED_VARIABLE_NAME_PREFIX) {
+            // Unknown ZED variable - use default if available
             if !default.is_empty() {
-                name.push_str(default);
+                // Strip the colon and return the default value
+                return Ok(Some(default[1..].to_owned()));
+            } else {
+                bail!("Unknown variable name: {variable_name}");
             }
-            return Ok(Some(name));
-        } else if variable_name.starts_with(ZED_VARIABLE_NAME_PREFIX) {
-            bail!("Unknown variable name: {variable_name}");
         }
         // This is an unknown variable.
         // We should not error out, as they may come from user environment (e.g. $PATH). That means that the variable substitution might not be perfect.
@@ -892,4 +893,81 @@ mod tests {
             "overwritten"
         );
     }
+
+    #[test]
+    fn test_variable_default_values() {
+        let task_with_defaults = TaskTemplate {
+            label: "test with defaults".to_string(),
+            command: format!(
+                "echo ${{{}}}",
+                VariableName::File.to_string() + ":fallback.txt"
+            ),
+            args: vec![
+                "${ZED_MISSING_VAR:default_value}".to_string(),
+                format!("${{{}}}", VariableName::Row.to_string() + ":42"),
+            ],
+            ..TaskTemplate::default()
+        };
+
+        // Test 1: When ZED_FILE exists, should use actual value and ignore default
+        let context_with_file = TaskContext {
+            cwd: None,
+            task_variables: TaskVariables::from_iter(vec![
+                (VariableName::File, "actual_file.rs".to_string()),
+                (VariableName::Row, "123".to_string()),
+            ]),
+            project_env: HashMap::default(),
+        };
+
+        let resolved = task_with_defaults
+            .resolve_task(TEST_ID_BASE, &context_with_file)
+            .expect("Should resolve task with existing variables");
+
+        assert_eq!(
+            resolved.resolved.command.unwrap(),
+            "echo actual_file.rs",
+            "Should use actual ZED_FILE value, not default"
+        );
+        assert_eq!(
+            resolved.resolved.args,
+            vec!["default_value", "123"],
+            "Should use default for missing var, actual value for existing var"
+        );
+
+        // Test 2: When ZED_FILE doesn't exist, should use default value
+        let context_without_file = TaskContext {
+            cwd: None,
+            task_variables: TaskVariables::from_iter(vec![(VariableName::Row, "456".to_string())]),
+            project_env: HashMap::default(),
+        };
+
+        let resolved = task_with_defaults
+            .resolve_task(TEST_ID_BASE, &context_without_file)
+            .expect("Should resolve task using default values");
+
+        assert_eq!(
+            resolved.resolved.command.unwrap(),
+            "echo fallback.txt",
+            "Should use default value when ZED_FILE is missing"
+        );
+        assert_eq!(
+            resolved.resolved.args,
+            vec!["default_value", "456"],
+            "Should use defaults for missing vars"
+        );
+
+        // Test 3: Missing ZED variable without default should fail
+        let task_no_default = TaskTemplate {
+            label: "test no default".to_string(),
+            command: "${ZED_MISSING_NO_DEFAULT}".to_string(),
+            ..TaskTemplate::default()
+        };
+
+        assert!(
+            task_no_default
+                .resolve_task(TEST_ID_BASE, &TaskContext::default())
+                .is_none(),
+            "Should fail when ZED variable has no default and doesn't exist"
+        );
+    }
 }