fix: validate include 'taskfile' field, suggest 'vars:' if missing#2860
Open
RichardHopperProGrammar wants to merge 1 commit into
Open
Conversation
When a user accidentally uses 'includes:' with variable-like content
(e.g., 'GOBIN: {sh: ...}') instead of 'vars:', the previous behavior
was to silently accept it and later report a misleading 'include cycle
detected' error, since the empty Taskfile path resolved to the same file.
This fix validates that each include entry has a non-empty 'taskfile'
field during YAML parsing and returns a clear, actionable error message
suggesting the user may have meant to use 'vars:' instead.
Closes go-task#1881
Contributor
|
See #1902 |
Author
Contributor
|
@RichardHopperProGrammar I think the other PR is dormant .... so you might prefer to keep yours open, and incorporate the feedback. Small concise changes that don't change too much have a good chance of being accepted. |
Author
Author
|
Thanks @trulede — I see #1902 is dormant so I will keep mine open and incorporate feedback. The test for missing taskfile vs. cycle error is already included (line 1069 in task_test.go — verifies the error says "taskfile not found" and not "include cycle detected"). Happy to make any other refinements you suggest. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a user accidentally writes
includes:with variable-like content instead ofvars:— a common typo like this:...the current behavior is to silently accept the malformed include entry (since any YAML mapping passes the
yaml.MappingNodecheck inIncludes.UnmarshalYAML), and later report a misleading "include cycle detected between A <--> A" error, because the emptyTaskfilepath resolves to the same file.This is confusing and unhelpful — the real problem isn't a cycle, it's that the user put variables under
includes:instead ofvars:.Solution
Added validation in
taskfile/ast/include.goto check that each include entry has a non-emptyTaskfilefield after decoding. If it doesn't:include "GOBIN" is missing the required "taskfile" field; did you mean to use "vars:" instead?sh) so the user understands what went wrongvars:instead)Changes
taskfile/ast/include.go: Added validation inIncludes.UnmarshalYAMLafter decoding each include value, checking for emptyTaskfileand producing a helpful error messagetestdata/includes_missing_taskfile/: Test fixtures reproducing the exact scenario from issue Incorrect/confusinginclude cycle detectederror message after a silly mistake #1881task_test.go: New testTestIncludesMissingTaskfileverifying the new error message is shown instead of the misleading cycle errorTesting
TestIncludes,TestIncludeCycle,TestIncludesIncorrect, etc.)