chore: addresses PR review comments

Author: nxtcoder17

Runfile.yml

@@ -1,5 +1,7 @@
 tasks:
   build:
+    env:
+      CGO_ENABLED: 0
     cmd: 
       - |+
         echo "building ..."
@@ -43,5 +45,5 @@ tasks:
   test:only-failing:
     cmd:
       - go test -json ./pkg/runfile/resolver/... | gotestfmt --hide successful-tests
-        go test -json ./pkg/executor/... $pattern_args | gotestfmt --hide successful-tests
+      - go test -json ./pkg/executor/... | gotestfmt --hide successful-tests
 

cmd/run/completions/run.fish

@@ -26,6 +26,7 @@ function __fish_completion_no_subcommand --description 'Test if there has been a
     return 0
 end
 
-complete -c completion -n '__fish_completion_no_subcommand' -f -l help -s h -d 'show help'
-complete -c completion -n '__fish_completion_no_subcommand' -f -l help -s h -d 'show help'
+complete -c run -n '__fish_completion_no_subcommand' -f -l help -s h -d 'show help'
+complete -c run -n '__fish_completion_no_subcommand' -f -l list -s l -d 'list all tasks'
+complete -c run -n '__fish_completion_no_subcommand' -rF -l file -s f -d 'runs targets from this runfile'
 

cmd/run/main.go

@@ -179,16 +179,6 @@ func main() {
 			// INFO: for supporting flags that have been suffixed post arguments
 			args := make([]string, 0, len(c.Args().Slice()))
 			for _, arg := range c.Args().Slice() {
-				if arg == "-p" || arg == "--parallel" {
-					parallel = true
-					continue
-				}
-
-				if arg == "-w" || arg == "--watch" {
-					watch = true
-					continue
-				}
-
 				if arg == "--debug" {
 					debug = true
 					continue
@@ -260,7 +250,7 @@ func getRunfilePath(dir string) (string, error) {
 		}
 
 		if stat.IsDir() {
-			return "", fmt.Errorf("Runfile.yml is a directory")
+			return "", fmt.Errorf("%s is a directory", filepath.Join(dir, f))
 		}
 
 		return filepath.Join(dir, f), nil
@@ -283,10 +273,15 @@ func locateRunfile(c *cli.Command) (string, error) {
 
 		for oldDir != dir {
 			fp, err := getRunfilePath(dir)
-			if err != nil && !errors.Is(err, ErrRunfileNotFound) {
-				oldDir = dir
-				dir = filepath.Dir(dir)
-				continue
+			if err != nil {
+				if errors.Is(err, ErrRunfileNotFound) {
+					// Not found in this dir, try parent
+					oldDir = dir
+					dir = filepath.Dir(dir)
+					continue
+				}
+				// Some other error
+				return "", err
 			}
 
 			return fp, nil

pkg/errors/constants.go

@@ -71,5 +71,5 @@ func ErrInvalidShellAlias(alias string) *errors.Error {
 }
 
 func ErrCircularDependency(taskName string) *errors.Error {
-	return errors.New("invalid shell alias").KV("task", taskName)
+	return errors.New("circular dependency detected").KV("task", taskName)
 }

pkg/runfile/resolver/env.go

@@ -16,21 +16,24 @@ import (
 )
 
 func parseDotEnvFilesInto(store map[string]string, files []string) error {
-	for _, f := range files {
-		if !filepath.IsAbs(f) {
-			return errors.New("dotenv file must have absolute paths").KV("dotenv.file", f)
+	for _, file := range files {
+		if !filepath.IsAbs(file) {
+			return errors.New("dotenv file must have absolute paths").KV("dotenv.file", file)
 		}
 
-		f, err := os.Open(f)
+		f, err := os.Open(file)
 		if err != nil {
-			return errors.New("failed to open dotenv file").Wrap(err).KV("dotenv.file", f)
+			return errors.New("failed to open dotenv file").Wrap(err).KV("dotenv.file", file)
 		}
 
 		m, err := godotenv.Parse(f)
 		if err != nil {
-			return errors.New("failed to parse dotenv file").Wrap(err).KV("dotenv.file", f)
+			return errors.New("failed to parse dotenv file").Wrap(err).KV("dotenv.file", file)
+		}
+
+		if err := f.Close(); err != nil {
+			return errors.New("failed to close dotenv file").Wrap(err).KV("dotenv.file", file)
 		}
-		f.Close()
 
 		maps.Copy(store, m)
 	}
@@ -70,7 +73,9 @@ func parseEnvInto(ctx context.Context, envStore map[string]string, envMap map[st
 						return errors.New("ENV-EXPRESSION: value field `required` must be a boolean").KV("env.key", k, "env.value", value)
 					}
 					if isRequired {
-						return errors.New(fmt.Sprintf("ENV-EXPRESSION: env var '%s' is required, it must be provided", k)).KV("env.key", k, "env.value", value)
+						if _, exists := lookupEnv(k); !exists {
+							return errors.New(fmt.Sprintf("ENV-EXPRESSION: env var '%s' is required, it must be provided", k)).KV("env.key", k, "env.value", value)
+						}
 					}
 				}
 
@@ -81,7 +86,10 @@ func parseEnvInto(ctx context.Context, envStore map[string]string, envMap map[st
 							return errors.New(fmt.Sprintf("ENV-EXPRESSION: value field `%s`, must have a string value", optKey)).KV("env.key", k, "env.value", value)
 						}
 
-						lazyEvalMap[k] = exec.CommandContext(ctx, shell[0], append(shell[1:], envEvalScript)...)
+					// #nosec G204 - This is intentional: env vars with sh/bash keys are meant to
+					// execute shell commands defined in the Runfile. The Runfile is trusted
+					// user-provided configuration, similar to Makefiles or shell scripts.
+					lazyEvalMap[k] = exec.CommandContext(ctx, shell[0], append(shell[1:], envEvalScript)...)
 						break
 					}
 				}

pkg/runfile/resolver/env_test.go

@@ -2,6 +2,7 @@ package resolver
 
 import (
 	"context"
+	"fmt"
 	"maps"
 	"os"
 	"path/filepath"
@@ -11,8 +12,7 @@ import (
 func TestParseDotEnvFilesInto(t *testing.T) {
 	tests := []struct {
 		name           string
-		setupFiles     map[string]string // filename -> content
-		files          []string          // will be converted to absolute paths
+		fileContents   []string // each entry becomes .env0, .env1, etc.
 		initialStore   map[string]string
 		expected       map[string]string
 		wantErr        bool
@@ -21,14 +21,13 @@ func TestParseDotEnvFilesInto(t *testing.T) {
 	}{
 		{
 			name:         "when files list is empty, it must pass",
-			files:        []string{},
+			fileContents: []string{},
 			initialStore: map[string]string{},
 			expected:     map[string]string{},
 			wantErr:      false,
 		},
 		{
 			name:       "when file path is relative, it must fail",
-			files:      []string{"relative/path/.env"},
 			useRelPath: true,
 			wantErr:    true,
 		},
@@ -37,15 +36,20 @@ func TestParseDotEnvFilesInto(t *testing.T) {
 			useNonExistent: true,
 			wantErr:        true,
 		},
+		{
+			name:         "when dotenv file has invalid syntax, it must return a parse error",
+			fileContents: []string{"INVALID LINE WITHOUT EQUALS\nANOTHER BAD LINE"},
+			initialStore: map[string]string{},
+			expected:     map[string]string{},
+			wantErr:      true,
+		},
 		{
 			name: "when file is valid, it must parse all key-value pairs",
-			setupFiles: map[string]string{
-				".env": `KEY1=value1
-								 KEY2=value2
-								 KEY3="quoted value"
-								 `,
+			fileContents: []string{
+				`KEY1=value1
+KEY2=value2
+KEY3="quoted value"`,
 			},
-			files:        []string{".env"},
 			initialStore: map[string]string{},
 			expected: map[string]string{
 				"KEY1": "value1",
@@ -56,13 +60,10 @@ func TestParseDotEnvFilesInto(t *testing.T) {
 		},
 		{
 			name: "when multiple files have same key, it must use value from last file",
-			setupFiles: map[string]string{
-				".env1": `KEY1=value1
-KEY2=original`,
-				".env2": `KEY2=overridden
-KEY3=value3`,
+			fileContents: []string{
+				"KEY1=value1\nKEY2=original",
+				"KEY2=overridden\nKEY3=value3",
 			},
-			files:        []string{".env1", ".env2"},
 			initialStore: map[string]string{},
 			expected: map[string]string{
 				"KEY1": "value1",
@@ -72,11 +73,8 @@ KEY3=value3`,
 			wantErr: false,
 		},
 		{
-			name: "when store has existing keys, it must preserve non-overlapping values",
-			setupFiles: map[string]string{
-				".env": `NEW_KEY=new_value`,
-			},
-			files:        []string{".env"},
+			name:         "when store has existing keys, it must preserve non-overlapping values",
+			fileContents: []string{"NEW_KEY=new_value"},
 			initialStore: map[string]string{"EXISTING": "existing_value"},
 			expected: map[string]string{
 				"EXISTING": "existing_value",
@@ -91,19 +89,18 @@ KEY3=value3`,
 			var files []string
 
 			if tt.useRelPath {
-				files = tt.files
+				files = []string{"relative/path/.env"}
 			} else if tt.useNonExistent {
 				files = []string{"/non/existent/path/.env"}
 			} else {
 				tmpDir := t.TempDir()
-				for filename, content := range tt.setupFiles {
+				for i, content := range tt.fileContents {
+					filename := fmt.Sprintf(".env%d", i)
 					path := filepath.Join(tmpDir, filename)
 					if err := os.WriteFile(path, []byte(content), 0644); err != nil {
 						t.Fatalf("failed to create temp file: %v", err)
 					}
-				}
-				for _, f := range tt.files {
-					files = append(files, filepath.Join(tmpDir, f))
+					files = append(files, path)
 				}
 			}
 

pkg/runfile/resolver/errors.go

@@ -107,7 +107,7 @@ func Load(ctx context.Context, file string) (*Resolver, error) {
 
 			for i := range task.DotEnv {
 				if !filepath.IsAbs(task.DotEnv[i]) {
-					task.DotEnv[i] = filepath.Join(runfileDir, task.DotEnv[i])
+					task.DotEnv[i] = filepath.Join(includedDir, task.DotEnv[i])
 				}
 			}
 

pkg/runfile/resolver/resolver.go

@@ -165,8 +165,6 @@ func (r *Resolver) RunTask(ctx context.Context, name string) error {
 		slog.Debug("3. watcher closed")
 	}()
 
-	pipeline.Start(ctx)
-
 	counter := 0
 	for ev := range watch.GetEvents() {
 		slog.Debug("received", "event", ev)

pkg/runfile/resolver/task.go

@@ -3,7 +3,6 @@ package resolver
 import (
 	"testing"
 
-	"github.com/nxtcoder17/runfile/pkg/executor"
 	"github.com/nxtcoder17/runfile/pkg/runfile/spec"
 	"github.com/nxtcoder17/runfile/pkg/writer"
 )
@@ -482,10 +481,3 @@ func TestCreateStepsWithEnv(t *testing.T) {
 		})
 	}
 }
-
-// Helper to check if a step uses the correct executor type
-func isInteractiveStep(step executor.Step) bool {
-	// This is a basic check - in real tests you might want to
-	// use reflection or add a method to identify command types
-	return len(step.Commands) > 0
-}