fix: avoid clobbering sh: vars when resolving global Ref values#2721
fix: avoid clobbering sh: vars when resolving global Ref values#2721JonZeolla wants to merge 3 commits intogo-task:mainfrom
Conversation
…bbering sh: vars The ReplaceVars call added in go-task#2632 re-processes all TaskfileVars after they've been resolved by rangeFunc. For sh: variables, this overwrites the already-resolved Value with a new Var that has the original Sh string but nil Value, causing dynamic variables to resolve to empty strings when the Taskfile has includes. Fix by only resolving variables that actually have Ref values set, leaving already-resolved sh: variables untouched. Fixes go-task#2720 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Temporarily reverting the compiler.go fix while keeping the test to demonstrate TDD — CI should fail on TestShVarWithIncludes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Re-applying the fix now that CI has demonstrated the test catches the regression. CI should pass on this commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a regression where global sh: vars could be overwritten during post-processing of global variables when a Taskfile contains includes:.
Changes:
- Adjusts global variable post-processing in
Compiler.getVariablesto only resolve globals withRefset. - Adds an executor test plus new fixture Taskfiles/golden output to cover
sh:vars with includes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| compiler.go | Changes global-var post-pass to resolve only Ref globals (avoiding sh: clobber). |
| executor_test.go | Adds TestShVarWithIncludes to exercise the regression scenario. |
| testdata/var_sh_with_includes/Taskfile.yml | New fixture Taskfile defining global sh: and derived vars plus an include. |
| testdata/var_sh_with_includes/Included.yml | Included fixture Taskfile to trigger include handling. |
| testdata/var_sh_with_includes/testdata/TestShVarWithIncludes-sh_var_not_clobbered_by_includes.golden | Golden output asserting sh: + derived var values remain correct. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| cache := &templater.Cache{Vars: result} | ||
| for k, v := range c.TaskfileVars.All() { | ||
| if v.Ref != "" { | ||
| c.TaskfileVars.Set(k, ast.Var{Value: templater.ResolveRef(v.Ref, cache)}) |
There was a problem hiding this comment.
This loop resolves Ref values by mutating c.TaskfileVars, but it never updates the result Vars map that getVariables returns and that is used for templating later in the same call. If a global ref: var failed to resolve in the first pass due to ordering (the regression fixed in #2632), it will still be nil/empty in result for the current execution. Consider also writing the resolved value back into result (and resetting the templater cache if needed), or performing a second evaluation pass that repopulates result from the updated global vars.
| c.TaskfileVars.Set(k, ast.Var{Value: templater.ResolveRef(v.Ref, cache)}) | |
| resolved := templater.ResolveRef(v.Ref, cache) | |
| c.TaskfileVars.Set(k, ast.Var{Value: resolved}) | |
| result.Set(k, ast.Var{Value: resolved}) |
Add TestShVarWithIncludes to ensure sh: dynamic variables are correctly resolved when a Taskfile also has includes. This guards against regressions like the one fixed in v3.49.1 (go-task#2720, go-task#2721). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
sh:variables resolve to empty strings when the Taskfile hasincludes:ReplaceVarscall added in fix: Call ReplaceVars() to resolve Ref's for imported global vars. #2632 now only processes variables withRefvalues set, leaving already-resolvedsh:variables untouchedsh:dynamic vars with included TaskfilesRoot Cause
The
ReplaceVarscall on line 118 ofcompiler.gore-processes allTaskfileVarsafter they've been resolved byrangeFunc. Forsh:variables,ReplaceVarWithExtrareturns a newVarwith the originalShstring butnilValue, destroying the already-resolved result.Test plan
TestShVarWithIncludeswith golden file assertingsh:vars resolve correctly alongside includesDERIVED_VAR= worldDERIVED_VAR=hello worldTestReferencesubtests still pass (includingref-globalfrom fix: Call ReplaceVars() to resolve Ref's for imported global vars. #2632)Fixes #2720