From a88b99ae56f31192d95ac7f14363eb767fe0dc60 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Tue, 8 Sep 2026 03:04:55 -0700 Subject: [PATCH] fix: resolve dotenv-declared {{.VAR}} chains in a deterministic order Motivation .env files loaded via `dotenv:` were parsed with godotenv.Read(), which returns a plain Go map[string]string. Task then copied that map into its own ordered variable store (ast.Vars) by ranging over it, which iterates in randomized order. Task's templater later resolves {{.VAR}} references between dotenv-declared variables in a single left-to-right pass over that store, so a variable like FINAL_DIR={{.MID_DIR}}/deeper could end up resolved before or after MID_DIR itself depending on the random iteration order, making the final value non-deterministic across runs. Taskfile- declared vars:/env: blocks were unaffected, since YAML already preserves their order. Approach Ported the parsing logic from github.com/joho/godotenv v1.5.1's parser.go into taskfile/dotenv.go, replacing its map[string]string accumulator with an ordered map (github.com/elliotchance/orderedmap/v3, already a dependency) so that dotenv variable order is preserved from file to ast.Vars. Both the global `dotenv:` field (taskfile/dotenv.go's Dotenv()) and the task-level `dotenv:` field (variables.go's compiledTask()) now use the new ReadDotenvOrdered() instead of godotenv.Read(), inserting variables into ast.Vars in file order. First-file-wins-on-duplicate-key semantics across multiple dotenv files is unchanged. The ported logic is attributed to its origin (MIT licensed) in a doc comment. Validation - go build ./... and go vet ./... pass. - go test ./... passes across all packages. - golangci-lint run ./... --new-from-rev=upstream/main reports 0 issues. - Added TestReadDotenvOrderedPreservesDeclarationOrder in taskfile/dotenv_test.go, parsing a dotenv file with export, quotes, comments, $VAR expansion and a duplicate key 20 times in a loop, asserting the resulting order and values are always identical. - Added TestDotenvNestedTemplateVarsResolveDeterministically in task_test.go, reproducing the issue's exact scenario (a 4-level {{.VAR}} chain declared in a dotenv file) and running the task 30 times, asserting the resolved output is always /home/user/nested/deeper/file.txt. - Confirmed this test is a real regression test: with the fix reverted (godotenv.Read() + map iteration restored), the same test fails intermittently (10 of 30 iterations in one run), producing wrong, partially-resolved output such as /nested/deeper/file.txt or /deeper/file.txt. With the fix applied, all 30 iterations pass, run after run. - Ran `gh run list --repo go-task/task --branch main --workflow=ci.yml --limit 10`: CI on main is currently green. Report: https://github.com/go-task/task/issues/1847 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- task_test.go | 21 ++ taskfile/dotenv.go | 255 +++++++++++++++++- taskfile/dotenv_test.go | 60 +++++ testdata/dotenv/nested_template_vars/.env | 4 + .../dotenv/nested_template_vars/Taskfile.yml | 8 + variables.go | 7 +- 6 files changed, 348 insertions(+), 7 deletions(-) create mode 100644 taskfile/dotenv_test.go create mode 100644 testdata/dotenv/nested_template_vars/.env create mode 100644 testdata/dotenv/nested_template_vars/Taskfile.yml diff --git a/task_test.go b/task_test.go index 540e7ba8de..a3596a0018 100644 --- a/task_test.go +++ b/task_test.go @@ -2223,6 +2223,27 @@ func TestDotenvHasEnvVarInPath(t *testing.T) { // nolint:paralleltest // cannot tt.Run(t) } +// TestDotenvNestedTemplateVarsResolveDeterministically is a regression test +// for https://github.com/go-task/task/issues/1847. Dotenv values used to be +// copied into Task's ordered vars by ranging over godotenv's plain +// map[string]string, which iterates in a randomized order. Chained +// {{.VAR}} references declared in a dotenv file would then only resolve in +// full on some runs, depending on that random order. Repeating the run many +// times gives the previous, order-dependent behavior many chances to fail. +func TestDotenvNestedTemplateVarsResolveDeterministically(t *testing.T) { // nolint:paralleltest // same output file is reused across iterations, which must run sequentially + tt := fileContentTest{ + Dir: "testdata/dotenv/nested_template_vars", + Target: "default", + TrimSpace: true, + Files: map[string]string{ + "full_path.txt": "/home/user/nested/deeper/file.txt", + }, + } + for range 30 { + tt.Run(t) + } +} + func TestTaskDotenvParseErrorMessage(t *testing.T) { t.Parallel() diff --git a/taskfile/dotenv.go b/taskfile/dotenv.go index a86a9eed1e..380d9fa9ff 100644 --- a/taskfile/dotenv.go +++ b/taskfile/dotenv.go @@ -1,10 +1,14 @@ package taskfile import ( + "bytes" "fmt" "os" + "regexp" + "strings" + "unicode" - "github.com/joho/godotenv" + "github.com/elliotchance/orderedmap/v3" "github.com/go-task/task/v3/internal/filepathext" "github.com/go-task/task/v3/internal/templater" @@ -26,11 +30,11 @@ func Dotenv(vars *ast.Vars, tf *ast.Taskfile, dir string) (*ast.Vars, error) { continue } - envs, err := godotenv.Read(dotEnvPath) + envs, err := ReadDotenvOrdered(dotEnvPath) if err != nil { return nil, fmt.Errorf("error reading env file %s: %w", dotEnvPath, err) } - for key, value := range envs { + for key, value := range envs.AllFromFront() { if _, ok := env.Get(key); !ok { env.Set(key, ast.Var{Value: value}) } @@ -39,3 +43,248 @@ func Dotenv(vars *ast.Vars, tf *ast.Taskfile, dir string) (*ast.Vars, error) { return env, nil } + +// ReadDotenvOrdered reads and parses a dotenv file, returning its variables in +// the order they are declared in the file. This is important because Task +// re-resolves each dotenv variable's value with its own {{.VAR}} templater, +// referencing the values of variables declared earlier in the same file. If +// the variables were provided out of order, a variable that depends on +// another declared above it in the file might resolve before its dependency +// does, depending on iteration order. +// +// The parsing logic below (through dotenvUnescapeCharsRegex) is adapted from +// github.com/joho/godotenv v1.5.1's parser.go, replacing its use of a plain +// map with an ordered map so that declaration order is preserved. +// +// godotenv is Copyright (c) 2013 John Barton and is distributed under the +// MIT License: https://github.com/joho/godotenv/blob/v1.5.1/LICENCE +func ReadDotenvOrdered(path string) (*orderedmap.OrderedMap[string, string], error) { + src, err := os.ReadFile(path) + if err != nil { + return nil, err + } + out := orderedmap.NewOrderedMap[string, string]() + if err := parseDotenvBytes(src, out); err != nil { + return nil, err + } + return out, nil +} + +const ( + dotenvCharComment = '#' + dotenvPrefixSingleQuote = '\'' + dotenvPrefixDoubleQuote = '"' + dotenvExportPrefix = "export" +) + +func parseDotenvBytes(src []byte, out *orderedmap.OrderedMap[string, string]) error { + src = bytes.ReplaceAll(src, []byte("\r\n"), []byte("\n")) + cutset := src + for { + cutset = dotenvStatementStart(cutset) + if cutset == nil { + break + } + + key, left, err := dotenvKeyName(cutset) + if err != nil { + return err + } + + value, left, err := dotenvVarValue(left, out) + if err != nil { + return err + } + + out.Set(key, value) + cutset = left + } + + return nil +} + +// dotenvStatementStart returns the position of the next statement, skipping +// any comment lines or leading whitespace. +func dotenvStatementStart(src []byte) []byte { + pos := bytes.IndexFunc(src, func(r rune) bool { return !unicode.IsSpace(r) }) + if pos == -1 { + return nil + } + + src = src[pos:] + if src[0] != dotenvCharComment { + return src + } + + pos = bytes.IndexRune(src, '\n') + if pos == -1 { + return nil + } + + return dotenvStatementStart(src[pos:]) +} + +// dotenvKeyName locates and parses a key name and returns the rest of the slice. +func dotenvKeyName(src []byte) (key string, cutset []byte, err error) { + src = bytes.TrimLeftFunc(src, dotenvIsSpace) + if trimmed, ok := bytes.CutPrefix(src, []byte(dotenvExportPrefix)); ok { + if bytes.IndexFunc(trimmed, dotenvIsSpace) == 0 { + src = bytes.TrimLeftFunc(trimmed, dotenvIsSpace) + } + } + + offset := 0 +loop: + for i, char := range src { + rchar := rune(char) + if dotenvIsSpace(rchar) { + continue + } + + switch char { + case '=', ':': + key = string(src[0:i]) + offset = i + 1 + break loop + case '_': + default: + if unicode.IsLetter(rchar) || unicode.IsNumber(rchar) || rchar == '.' { + continue + } + return "", nil, fmt.Errorf( + `unexpected character %q in variable name near %q`, + string(char), string(src)) + } + } + + if len(src) == 0 { + return "", nil, fmt.Errorf("zero length string") + } + + key = strings.TrimRightFunc(key, unicode.IsSpace) + cutset = bytes.TrimLeftFunc(src[offset:], dotenvIsSpace) + return key, cutset, nil +} + +// dotenvVarValue extracts a variable value and returns the rest of the slice. +func dotenvVarValue(src []byte, vars *orderedmap.OrderedMap[string, string]) (value string, rest []byte, err error) { + quote, hasPrefix := dotenvQuotePrefix(src) + if !hasPrefix { + endOfLine := bytes.IndexFunc(src, dotenvIsLineEnd) + + if endOfLine == -1 { + endOfLine = len(src) + if endOfLine == 0 { + return "", nil, nil + } + } + + line := []rune(string(src[0:endOfLine])) + + endOfVar := len(line) + if endOfVar == 0 { + return "", src[endOfLine:], nil + } + + for i := endOfVar - 1; i >= 0; i-- { + if line[i] == dotenvCharComment && i > 0 { + if dotenvIsSpace(line[i-1]) { + endOfVar = i + break + } + } + } + + trimmed := strings.TrimFunc(string(line[0:endOfVar]), dotenvIsSpace) + + return dotenvExpandVariables(trimmed, vars), src[endOfLine:], nil + } + + for i := 1; i < len(src); i++ { + if char := src[i]; char != quote { + continue + } + + if prevChar := src[i-1]; prevChar == '\\' { + continue + } + + trimFunc := func(r rune) bool { return r == rune(quote) } + value = string(bytes.TrimLeftFunc(bytes.TrimRightFunc(src[0:i], trimFunc), trimFunc)) + if quote == dotenvPrefixDoubleQuote { + value = dotenvExpandVariables(dotenvExpandEscapes(value), vars) + } + + return value, src[i+1:], nil + } + + valEndIndex := bytes.IndexRune(src, '\n') + if valEndIndex == -1 { + valEndIndex = len(src) + } + + return "", nil, fmt.Errorf("unterminated quoted value %s", src[:valEndIndex]) +} + +func dotenvExpandEscapes(str string) string { + out := dotenvEscapeRegex.ReplaceAllStringFunc(str, func(match string) string { + c := match[1:] + switch c { + case "n": + return "\n" + case "r": + return "\r" + default: + return match + } + }) + return dotenvUnescapeCharsRegex.ReplaceAllString(out, "$1") +} + +func dotenvExpandVariables(v string, m *orderedmap.OrderedMap[string, string]) string { + return dotenvExpandVarRegex.ReplaceAllStringFunc(v, func(s string) string { + submatch := dotenvExpandVarRegex.FindStringSubmatch(s) + if submatch == nil { + return s + } + if submatch[1] == "\\" || submatch[2] == "(" { + return submatch[0][1:] + } else if submatch[4] != "" { + return m.GetOrDefault(submatch[4], "") + } + return s + }) +} + +func dotenvQuotePrefix(src []byte) (prefix byte, isQuoted bool) { + if len(src) == 0 { + return 0, false + } + switch prefix := src[0]; prefix { + case dotenvPrefixDoubleQuote, dotenvPrefixSingleQuote: + return prefix, true + default: + return 0, false + } +} + +// dotenvIsSpace reports whether the rune is a space character but not a line +// break character. This differs from unicode.IsSpace, which also treats line +// breaks as space. +func dotenvIsSpace(r rune) bool { + switch r { + case '\t', '\v', '\f', '\r', ' ', 0x85, 0xA0: + return true + } + return false +} + +func dotenvIsLineEnd(r rune) bool { + return r == '\n' || r == '\r' +} + +var ( + dotenvEscapeRegex = regexp.MustCompile(`\\.`) + dotenvExpandVarRegex = regexp.MustCompile(`(\\)?(\$)(\()?\{?([A-Z0-9_]+)?\}?`) + dotenvUnescapeCharsRegex = regexp.MustCompile(`\\([^$])`) +) diff --git a/taskfile/dotenv_test.go b/taskfile/dotenv_test.go new file mode 100644 index 0000000000..8f409123de --- /dev/null +++ b/taskfile/dotenv_test.go @@ -0,0 +1,60 @@ +package taskfile + +import ( + "os" + "path/filepath" + "slices" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReadDotenvOrderedPreservesDeclarationOrder(t *testing.T) { + t.Parallel() + + content := `# a comment +export BASE_DIR=/home/user +MID_DIR={{.BASE_DIR}}/nested +FINAL_DIR="{{.MID_DIR}}/deeper" +FULL_PATH=$FINAL_DIR/file.txt + +QUOTED='single quoted' +FULL_PATH=$FINAL_DIR/overridden.txt +` + + dir := t.TempDir() + path := filepath.Join(dir, ".env") + require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + + // Run several times to make sure the order is always the same. This + // mirrors the previous godotenv.Read-based implementation, which built + // its result in a plain Go map and iterated over it in random order. + for range 20 { + envs, err := ReadDotenvOrdered(path) + require.NoError(t, err) + + assert.Equal(t, + []string{"BASE_DIR", "MID_DIR", "FINAL_DIR", "FULL_PATH", "QUOTED"}, + slices.Collect(envs.Keys()), + ) + + baseDir, _ := envs.Get("BASE_DIR") + assert.Equal(t, "/home/user", baseDir) + + midDir, _ := envs.Get("MID_DIR") + assert.Equal(t, "{{.BASE_DIR}}/nested", midDir) + + finalDir, _ := envs.Get("FINAL_DIR") + assert.Equal(t, "{{.MID_DIR}}/deeper", finalDir) + + // Last declaration of a duplicate key wins, but keeps its original + // position in the order. $FINAL_DIR is expanded eagerly, using + // FINAL_DIR's raw (not yet Go-template-expanded) value. + fullPath, _ := envs.Get("FULL_PATH") + assert.Equal(t, "{{.MID_DIR}}/deeper/overridden.txt", fullPath) + + quoted, _ := envs.Get("QUOTED") + assert.Equal(t, "single quoted", quoted) + } +} diff --git a/testdata/dotenv/nested_template_vars/.env b/testdata/dotenv/nested_template_vars/.env new file mode 100644 index 0000000000..4ff31c8137 --- /dev/null +++ b/testdata/dotenv/nested_template_vars/.env @@ -0,0 +1,4 @@ +BASE_DIR=/home/user +MID_DIR={{.BASE_DIR}}/nested +FINAL_DIR={{.MID_DIR}}/deeper +FULL_PATH={{.FINAL_DIR}}/file.txt diff --git a/testdata/dotenv/nested_template_vars/Taskfile.yml b/testdata/dotenv/nested_template_vars/Taskfile.yml new file mode 100644 index 0000000000..455bbb0b16 --- /dev/null +++ b/testdata/dotenv/nested_template_vars/Taskfile.yml @@ -0,0 +1,8 @@ +version: '3' + +dotenv: ['.env'] + +tasks: + default: + cmds: + - echo "$FULL_PATH" > full_path.txt diff --git a/variables.go b/variables.go index c2085bd1ea..002c5b86c8 100644 --- a/variables.go +++ b/variables.go @@ -7,8 +7,6 @@ import ( "path/filepath" "strings" - "github.com/joho/godotenv" - "github.com/go-task/task/v3/errors" "github.com/go-task/task/v3/internal/deepcopy" "github.com/go-task/task/v3/internal/env" @@ -16,6 +14,7 @@ import ( "github.com/go-task/task/v3/internal/filepathext" "github.com/go-task/task/v3/internal/fingerprint" "github.com/go-task/task/v3/internal/templater" + "github.com/go-task/task/v3/taskfile" "github.com/go-task/task/v3/taskfile/ast" ) @@ -177,11 +176,11 @@ func (e *Executor) compiledTask(call *Call, evaluateShVars bool) (*ast.Task, err if _, err := os.Stat(dotEnvPath); os.IsNotExist(err) { continue } - envs, err := godotenv.Read(dotEnvPath) + envs, err := taskfile.ReadDotenvOrdered(dotEnvPath) if err != nil { return nil, err } - for key, value := range envs { + for key, value := range envs.AllFromFront() { if _, ok := dotenvEnvs.Get(key); !ok { dotenvEnvs.Set(key, ast.Var{Value: value}) }