fix: resolve enum refs that depend on dynamic variables in interactive prompts - #2969
fix: resolve enum refs that depend on dynamic variables in interactive prompts#2969ikshantshukla123 wants to merge 3 commits into
Conversation
The previous fix for go-task#2958 missed the fast-compiled var shape: FastGetVariables stores un-evaluated dynamic variables as {Value: "", Sh: ...}, so the `newVar.Value != nil` check in evaluateDynamicVarsForPrompt treated them as static and never ran HandleDynamicVar. The enum ref resolved to an empty list and the prompt fell back to free-form input. Only treat a variable as static when it has a non-nil value and no shell command. Add a regression test using the fast-compiled var shape so this cannot regress again.
|
@trulede have a look. |
| vCopy := v.DeepCopy() | ||
| cache := &templater.Cache{Vars: vars} | ||
| _ = resolveEnumRefs(&ast.Requires{Vars: []*ast.VarsWithValidation{vCopy}}, cache) | ||
| if err := resolveEnumRefs(&ast.Requires{Vars: []*ast.VarsWithValidation{vCopy}}, cache); err == nil && len(vCopy.Enum.Value) > 0 { |
There was a problem hiding this comment.
There is a lot of duplicated logic. I would try to streamline it. Evaluate vars, then resolve enum refs, done.
There was a problem hiding this comment.
And see about writing tests similar to this if possible:
https://github.com/go-task/task/tree/main/testdata/prompt
Because AI generated tests are next-to-useless.
There was a problem hiding this comment.
Done. resolveEnumRefForPrompt is now: evaluate dynamic vars first (when any exist), then resolve the enum ref once — no more two-pass retry or duplicated resolveEnumRefs/DeepCopy.
For tests, since the interactive prompt is bubbletea-based (can't be driven by a plain stdin buffer in the harness, same reason the existing testdata/prompt tests use the prompt: field instead of requires:), I added a fixture-based integration test at testdata/enum_ref_prompt that drives the real prompt path (FastCompiledTask → resolveEnumRefForPrompt) and asserts the resolved enum values, plus targeted unit tests for the edge cases (empty dynamic output, failing unrelated sh: var).
There was a problem hiding this comment.
And see about writing tests similar to this if possible: https://github.com/go-task/task/tree/main/testdata/prompt
Because AI generated tests are next-to-useless.
Added fixture-based tests at testdata/enum_ref_prompt/ that run the real prompt path against a Taskfile (dynamic sh: var, static list var, and an empty dynamic var), added in TestPromptEnumRefResolution. The existing unit tests in requires_internal_test.go were reworked to assert the actual resolved enum values for realistic scenarios.
Note: a fully end-to-end golden test like testdata/prompt isn't possible here because the requires: prompt uses a bubbletea-based input.Prompter, which can't be driven by the plain-stdin test harness (only the prompt: confirm field can).
There was a problem hiding this comment.
@trulede Ready for rereview whenever you get a chance
Simplify resolveEnumRefForPrompt per review feedback: evaluate dynamic (sh:) variables first when present, then resolve the enum ref once, instead of a two-pass resolve-and-retry that duplicated resolveEnumRefs and DeepCopy. Replace the hand-rolled unit fixtures with a fixture-based integration test that drives the real prompt path (FastCompiledTask + resolveEnumRefForPrompt) against testdata/enum_ref_prompt, plus targeted unit tests for edge cases: empty dynamic output and a failing unrelated sh: var.
| if vars == nil { | ||
| return false | ||
| } | ||
| for v := range vars.Values() { |
There was a problem hiding this comment.
Should this be for _, v ...
There was a problem hiding this comment.
and if wrong, perhaps why the unit test does not fail.
There was a problem hiding this comment.
and if wrong, perhaps why the unit test does not fail.
Good question vars.Values() returns iter.Seq[Var] (a single-value iterator, not (k, v) pairs), so for v := range vars.Values() yields each Var directly. for _, v := range wouldn't compile here since iter.Seq[Var] ranges over exactly one variable.
On the test question it's guarded by TestPromptEnumRefResolution — if hasDynamicVars returned false for the dynamic sh: fixture, the enum would come back empty and that test would fail. (And hasDynamicVars is really just a cheap guard; evaluateDynamicVarsForPrompt handles the no-op case fine either way.)
|
@trulede thanks for being so patient. @ikshantshukla123 please refer to our policy on AI. It's clear that an LLM is posting with your account. It looks like you're pasting trulede's answers to an LLM and then pasting the response back into GitHub (with the associated commit, also generated) Please don't do that. |
Sorry sir but it was not blindly copy pasted , my English is weak so with my pointers I used ai to generate convo. Can you suggest another method? |
@trulede sir can you give some tips? |
|
Yes. Don't paste crap from AI, its always junk. Unit tests written by AI are junk too .... and the project has to maintain that ... which is actually quite some effort. You simply write that: Use AI to help you, but write the code yourself. And the PR too. |
Fixes #2958
Summary
task --interactiveshowed a free-form text prompt (instead of a selection menu) when anenum.refpointed to a dynamicsh:variable. The previous enum-ref support (#2927) resolved refs against the fast-compiled task vars, wheresh:variables are not evaluated and resolve to"". The ref then produced an empty list and the prompter fell back toTextinput.This PR evaluates the relevant dynamic (
sh:) variables first, so the enum ref resolves to the real values and aSelectmenu is shown.Changes
resolveEnumRefForPromptnow evaluates dynamic (sh:) variables before resolving the enum ref, then resolves the ref a single time. It reuses the compiler's dynamic-var cache and mirrorsGetVariablesevaluation order, and a failing command is left unevaluated so the remaining variables can still resolve.FastGetVariablesstores un-evaluated dynamic vars as{Value: "", Sh: ...}(a non-nil empty value), so treating any non-nil value as static meant thesh:command never ran.go build ./...,go vet ./...,golangci-lint run ./..., and the full root-package test suite.Tests
TestPromptEnumRefResolutiondriving the real prompt path (FastCompiledTask→resolveEnumRefForPrompt) againsttestdata/enum_ref_prompt/, covering a dynamicsh:ref, a static list ref, and an empty dynamic var.resolveEnumRefForPromptunit tests to assert the actual resolved enum values for realistic scenarios (fast-compiled var shape, empty dynamic output, failing unrelatedsh:var).Note: a golden end-to-end test like
testdata/promptisn't possible for this path because therequires:prompt uses a bubbletea-basedinput.Prompter, which can't be driven by the plain-stdin test harness (only theprompt:confirm field can).AI usage disclosure
I used DeepSeek (via the opencode CLI) to understand the fast-compiled var shape and the templater resolution flow, and to help write the regression tests included in this PR. The duplicate-logic review feedback was applied manually; the streamlined flow is written by hand.