Centralize interactive huh form setup in pkg/console and remove CLI duplication#43587
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pkg/console and remove CLI duplication
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR centralizes huh form construction behind pkg/console helpers so terminal theme and accessibility defaults are applied consistently, and migrates existing interactive CLI call sites away from repeated .WithTheme(...).WithAccessible(...) chains.
Changes:
- Added non-WASM
pkg/consolehelpers to construct themed, accessibility-aware*huh.Forminstances. - Migrated interactive prompts across
pkg/cli(and existingpkg/consoleprompts) to use the new wrappers. - Added a small wrapper test to ensure wrapper constructors return non-nil forms.
Show a summary per file
| File | Description |
|---|---|
| pkg/console/prompt_form.go | Adds centralized huh form construction wrappers with default theme + accessibility settings. |
| pkg/console/prompt_form_test.go | Adds basic unit test coverage for the new wrapper constructors. |
| pkg/console/list.go | Uses the new wrapper instead of locally applying theme/accessibility on a huh.NewForm(...). |
| pkg/console/input.go | Uses the new wrapper for secret input form construction. |
| pkg/console/confirm.go | Uses the new wrapper for confirmation form construction. |
| pkg/cli/run_interactive.go | Replaces inline huh.NewForm(...).WithTheme(...).WithAccessible(...) with console.Prompt* wrappers. |
| pkg/cli/interactive.go | Uses console.PromptForm / console.PromptInput / console.PromptConfirm for interactive workflow builder prompts. |
| pkg/cli/engine_secrets.go | Uses console.PromptInput wrapper for token/API key prompt forms. |
| pkg/cli/add_interactive_workflow.go | Uses console.PromptConfirm wrapper for “run now” confirmation. |
| pkg/cli/add_interactive_schedule.go | Uses console.PromptSelect wrapper for schedule frequency selection. |
| pkg/cli/add_interactive_orchestrator.go | Uses console.PromptConfirm wrapper for final confirmation. |
| pkg/cli/add_interactive_git.go | Uses wrappers for select + input prompts during PR handling steps. |
| pkg/cli/add_interactive_engine.go | Uses console.PromptSelect wrapper for engine/auth method selection. |
| pkg/cli/add_interactive_auth.go | Uses console.PromptInput wrapper for repository entry prompt. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Low
| // PromptInput creates a themed, accessibility-aware single-input form. | ||
| func PromptInput(input *huh.Input) *huh.Form { | ||
| return PromptForm(huh.NewGroup(input)) | ||
| } | ||
|
|
| return PromptForm(huh.NewGroup(selectField)) | ||
| } | ||
|
|
||
| // PromptMultiSelect creates a themed, accessibility-aware single-multiselect form. |
🧪 Test Quality Sentinel Report❌ Test Quality Score: 20/100 — Poor
📊 Metrics (1 test)
|
…pkg/console Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (224 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
Review SummaryApplied 📋 Findings1. Missing
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking issues, but two structural gaps worth fixing before this ships.
📋 Key Themes & Highlights
Key Themes
-
Missing WASM stubs —
prompt_form.goadds 6 new exported functions under//go:build !js && !wasmbut there is no companionprompt_form_wasm.go. Every other file inpkg/consolefollows thefile.go/file_wasm.gopairing (confirm, input, list, banner, ...). Without a stub the WASM build either fails to link or silently omits the symbols. -
Weak test contract —
TestPromptWrappersReturnNonNilFormsconfirms non-nil returns but does not verify the invariant the wrappers exist to enforce: that the returned form carries the theme and accessibility settings. The test would pass even if both.WithThemeand.WithAccessiblecalls were removed.
Positive Highlights
- ✅ Clean, well-scoped abstraction — the six wrappers cover exactly the field types used in
pkg/cliwithout over-generalising. - ✅ Consistent removal of
pkg/stylesimport across all 9 CLI files — no stray references left behind. - ✅ Updating the existing console helpers (
confirm.go,input.go,list.go) to use the new wrappers gives the package a single construction path. - ✅ Net -203 lines with no behaviour change — exactly the right scope for a de-duplication refactor.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 72.2 AIC · ⌖ 8.71 AIC · ⊞ 6.7K
Comment /matt to run again
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestPromptWrappersReturnNonNilForms(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The test only verifies the wrapper returns non-nil — it does not verify the core contract: that theme and accessibility settings are actually baked in.
💡 Suggestions for strengthening the contract test
The whole point of these wrappers is to guarantee .WithTheme(styles.HuhTheme) and .WithAccessible(IsAccessibleMode()) are applied. A non-nil check would pass even if the wrappers forgot those calls. Consider:
- Set accessible mode via env and verify propagation — e.g.,
t.Setenv("GH_AW_ACCESSIBLE", "1")then assert that a form built withPromptFormdiffers in behaviour from a rawhuh.NewFormin accessible mode. - Test the variadic
PromptFormwith multiple groups to exercise that path, which callsites likepromptForConfigurationrely on.
@copilot please address this.
| package console | ||
|
|
||
| import ( | ||
| "charm.land/huh/v2" |
There was a problem hiding this comment.
[/codebase-design] There is no WASM stub counterpart for the new PromptForm, PromptInput, PromptSelect, PromptMultiSelect, PromptText, and PromptConfirm functions. The existing pattern in this package is to pair every non-wasm file with a _wasm.go stub (see confirm.go/confirm_wasm.go, input.go/input_wasm.go, etc.).
💡 What's needed
Add a prompt_form_wasm.go with the //go:build js || wasm tag that returns an error (or panics) for each exported function. Without it, any WASM consumer that references these symbols will get a linker error.
(go/redacted):build js || wasm
package console
import "errors"
var errNotAvailableInWasm = errors.New("interactive forms not available in Wasm")
func PromptForm(groups ...*huh.Group) (*huh.Form, error) {
return nil, errNotAvailableInWasm
}
// ... etcCheck whether the existing wasm stubs panic or return errors to stay consistent.
@copilot please address this.
|
|
||
| // PromptSelect creates a themed, accessibility-aware single-select form. | ||
| func PromptSelect[T comparable](selectField *huh.Select[T]) *huh.Form { | ||
| return PromptForm(huh.NewGroup(selectField)) |
There was a problem hiding this comment.
[/codebase-design] PromptInput, PromptSelect, etc. are thin one-liner wrappers over PromptForm. That is fine, but the doc comment says "single-input form" / "single-select form" — which implies a constraint the implementation doesn't enforce. If a caller passes a field that already belongs to a group (which huh fields don't, but future callers may expect to pass a pre-grouped select), the implicit contract could cause confusion.
💡 Suggestion
Sharpen the doc comments to describe what the function does rather than the implied constraint:
// PromptInput wraps a single *huh.Input in a themed, accessible form.
// Use PromptForm for multi-group layouts.
func PromptInput(input *huh.Input) *huh.Form {This makes the scope of each helper explicit and helps future readers understand when to reach for PromptForm directly.
@copilot please address this.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestPromptWrappersReturnNonNilForms(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] TestPromptWrappersReturnNonNilForms bundles all five wrapper types into one test function. If one wrapper panics, the others are never exercised and the failure message doesn't identify which wrapper broke.
💡 Split into per-wrapper subtests
func TestPromptInput(t *testing.T) {
var v string
require.NotNil(t, PromptInput(huh.NewInput().Value(&v)))
}
func TestPromptSelect(t *testing.T) { ... }
// etc.or use t.Run subtests. This makes failures immediately actionable and test names self-documenting.
@copilot please address this.
There was a problem hiding this comment.
Review: Centralize interactive huh form setup
The DX improvement here is solid — removing repeated .WithTheme(...).WithAccessible(...) chains across pkg/cli is the right direction. The implementation is clean and the migration is consistent. One blocking issue needs to be resolved before merge.
🔴 Blocking: PromptInput/PromptSelect/PromptMultiSelect name collision with WASM stubs
prompt_form.go (!js && !wasm) introduces these three functions with *huh.Field → *huh.Form signatures, while input_wasm.go and select_wasm.go (js || wasm) already define identically-named functions with string-parameter signatures that return (T, error). Build tags keep them from conflicting at compile time, but:
- The public API is now build-target-dependent: same name, different arity, different return type.
pkg/console/README.mdstill documents all three as "only available in WASM builds" — directly contradicted by this PR.
Either rename the new wrappers to avoid collision, or retire the old wasm stubs and update the README to describe the new unified API.
🟡 Minor: Doc comment typo
"single-multiselect" → "single multi-select" on line 26 of prompt_form.go.
What's working well
- All 14 changed files consistently use the new wrappers — no partial migration.
prompt_form.gouses the!js && !wasmbuild tag, matching the pattern of all other non-wasm interactive files in the package.- The wrapper test covers all 5 new functions.
- Removing the
pkg/stylesimport from CLI files is a clean separation of concerns.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 115.8 AIC · ⌖ 5.29 AIC · ⊞ 4.9K
| } | ||
|
|
||
| // PromptInput creates a themed, accessibility-aware single-input form. | ||
| func PromptInput(input *huh.Input) *huh.Form { |
There was a problem hiding this comment.
The non-wasm PromptInput(*huh.Input) *huh.Form (and PromptSelect, PromptMultiSelect) share names with pre-existing WASM-only counterparts in input_wasm.go / select_wasm.go that have entirely different signatures ((string, string, string) (string, error) vs (*huh.Input) *huh.Form). Since these are gated by mutually exclusive build tags (!js && !wasm vs js || wasm), compilation succeeds — but this creates a build-target-dependent public API where the same symbol has different arities, types, and return values depending on the target.
This is a correctness hazard:
- The README still describes
PromptInput,PromptSelect, andPromptMultiSelectas "only available in WASM builds" (pkg/console/README.mdlines 612, 656, 674). This is now false for non-wasm builds and is directly contradicted by this PR. - Callers that know the package docs will be confused about which overload exists on their target platform.
Please either:
- Rename the new non-wasm helpers to avoid the naming clash (e.g.
NewThemedInput,NewThemedSelect), or - Explicitly retire/rename the wasm stubs and update
README.mdto document the new unified API.
@copilot please address this.
| } | ||
|
|
||
| // PromptMultiSelect creates a themed, accessibility-aware single-multiselect form. | ||
| func PromptMultiSelect[T comparable](multiSelect *huh.MultiSelect[T]) *huh.Form { |
There was a problem hiding this comment.
Doc comment says "single-multiselect" — this should be "single multi-select" (two words) to match standard Go/TUI terminology.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — three issues must be addressed before merging
The centralization goal is sound. The mechanical refactoring in pkg/cli is clean and complete. Three blocking problems remain:
🔴 Issues found
1. WASM build is broken (high) — prompt_form.go adds six exported functions under !js && !wasm, but input_wasm.go and select_wasm.go still declare PromptInput, PromptSelect, PromptMultiSelect with their old, incompatible signatures. PromptForm, PromptText, and PromptConfirm have no WASM stub at all. The WASM build target will have duplicate symbols with mismatched types, or outright missing symbols. See inline comment on prompt_form.go:1.
2. Tests are vacuously true (high) — require.NotNil on huh.NewForm(...) is always true; the PR's actual invariant (theme and accessibility are baked in) is completely untested. A future regression that removes WithTheme or WithAccessible passes silently. See inline comment on prompt_form_test.go:13.
3. PromptForm() zero-groups trap (medium) — the variadic signature accepts zero groups with no guard, producing a broken form at run time with no construction-time signal. See inline comment on prompt_form.go:12.
🔎 Code quality review by PR Code Quality Reviewer · 159.6 AIC · ⌖ 8.56 AIC · ⊞ 5.4K
Comment /review to run again
| @@ -0,0 +1,38 @@ | |||
| //go:build !js && !wasm | |||
There was a problem hiding this comment.
Incomplete WASM refactor: 3 new exported functions have no _wasm.go stubs, and 3 existing stubs now have mismatched signatures.
Every non-WASM file in pkg/console has a corresponding _wasm.go counterpart — this PR breaks that invariant:
PromptForm,PromptText,PromptConfirm— added here with noprompt_form_wasm.gostub. They simply don't exist in WASM builds.PromptInput,PromptSelect,PromptMultiSelect— redefined here with new signatures (*huh.Input → *huh.Form), butinput_wasm.go/select_wasm.gostill export the OLD signatures ((title, description, placeholder string) (string, error)).
There are now two unrelated PromptInput functions in this package that share a name but live in separate build contexts with incompatible signatures and return types. Any WASM caller that adds a dependency on these will get confusing compile failures, or silently call the wrong stub.
💡 Suggested fix
Add pkg/console/prompt_form_wasm.go with stubs for all six functions, and remove the old-signature stubs from input_wasm.go and select_wasm.go:
(go/redacted):build js || wasm
package console
import (
"charm.land/huh/v2"
"errors"
)
var errNotAvailable = errors.New("interactive forms not available in Wasm")
func PromptForm(groups ...*huh.Group) *huh.Form { panic(errNotAvailable) }
func PromptInput(input *huh.Input) *huh.Form { panic(errNotAvailable) }
func PromptSelect[T comparable](s *huh.Select[T]) *huh.Form { panic(errNotAvailable) }
func PromptMultiSelect[T comparable](m *huh.MultiSelect[T]) *huh.Form { panic(errNotAvailable) }
func PromptText(t *huh.Text) *huh.Form { panic(errNotAvailable) }
func PromptConfirm(c *huh.Confirm) *huh.Form { panic(errNotAvailable) }Also remove or rename the old-signature PromptInput, PromptSelect, PromptMultiSelect stubs from input_wasm.go and select_wasm.go to eliminate the silent signature split.
| ) | ||
|
|
||
| func TestPromptWrappersReturnNonNilForms(t *testing.T) { | ||
| var inputValue string |
There was a problem hiding this comment.
Tests verify construction never panics — not the actual invariant this PR exists to enforce.
require.NotNil(t, PromptInput(...)) is vacuously true: huh.NewForm() never returns nil. The PR's stated purpose is to guarantee .WithTheme(styles.HuhTheme).WithAccessible(IsAccessibleMode()) are applied on every form path. Neither property is asserted anywhere in this test file.
A regression that removes WithTheme or WithAccessible from PromptForm will pass these tests without complaint.
💡 Suggested fix
Test the actual contract. huh.Form exposes enough to check at least the accessibility setting:
func TestPromptFormAppliesAccessibility(t *testing.T) {
t.Setenv("ACCESSIBLE", "1") // or whatever env key IsAccessibleMode reads
var v string
form := PromptInput(huh.NewInput().Value(&v))
require.NotNil(t, form, "PromptInput must return a non-nil form")
// If huh.Form exposes an Accessible() getter, assert it:
// require.True(t, form.IsAccessible(), "form must have accessible mode enabled when ACCESSIBLE=1")
}
func TestPromptFormWrappersReturnNonNil(t *testing.T) {
for _, tc := range []struct{
name string
form *huh.Form
}{
{"PromptInput", PromptInput(huh.NewInput().Value(new(string)))},
{"PromptSelect", PromptSelect(huh.NewSelect[string]().Options(huh.NewOption("x","x")).Value(new(string)))},
{"PromptMultiSelect", PromptMultiSelect(huh.NewMultiSelect[string]().Options(huh.NewOption("x","x")).Value(new([]string)))},
{"PromptText", PromptText(huh.NewText().Value(new(string)))},
{"PromptConfirm", PromptConfirm(huh.NewConfirm().Value(new(bool)))},
} {
t.Run(tc.name, func(t *testing.T) {
require.NotNil(t, tc.form, "%s must return a non-nil form", tc.name)
})
}
}At minimum, use sub-tests so each wrapper failure is independently reported.
|
|
||
| // PromptForm creates a huh form with gh-aw's default theme and accessibility mode. | ||
| func PromptForm(groups ...*huh.Group) *huh.Form { | ||
| return huh.NewForm(groups...).WithTheme(styles.HuhTheme).WithAccessible(IsAccessibleMode()) |
There was a problem hiding this comment.
PromptForm() with zero groups is a compile-time-valid call that produces a broken form at runtime.
PromptForm is variadic and exported — console.PromptForm() compiles fine. huh.NewForm() with no groups results in a form that either panics or returns an unexpected error on Run(), with no hint at the call site. Given this is now the single centralized construction path, future callsites (especially PromptText/PromptConfirm users constructing groups manually) can easily call it with zero groups by mistake.
💡 Suggested fix
func PromptForm(groups ...*huh.Group) *huh.Form {
if len(groups) == 0 {
panic("PromptForm: at least one group is required")
}
return huh.NewForm(groups...).WithTheme(styles.HuhTheme).WithAccessible(IsAccessibleMode())
}Failing loudly at construction time is far easier to debug than a mysterious failure when the form is run.
🤖 PR Triage — Run §28750744185
Score breakdown: Impact 18 + Urgency 10 + Quality 10 Rationale: Large centralization refactor of
Recommended next step: Defer until review feedback is addressed. Then batch review with #43588 and #43589 as the console-migration cluster.
|
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…WASM naming collision Renames the non-WASM form construction helpers to avoid overloading the `Prompt*` namespace already used by WASM-only prompt APIs with different signatures/semantics: PromptForm → NewForm PromptInput → NewInputForm PromptSelect → NewSelectForm PromptMultiSelect→ NewMultiSelectForm PromptText → NewTextForm PromptConfirm → NewConfirmForm Also fixes doc comment typo: "single-multiselect" → "single multi-select". Updates all callsites in pkg/console and pkg/cli, and the ADR. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Renamed the non-WASM form construction helpers to Pre-existing unrelated failure: |
|
@copilot please run the
|
All review threads were resolved in the previous pass (commit |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
|
Run 28753588118 is a scheduled |
|
🎉 This pull request is included in a new release. Release: |
This change centralizes interactive
huhform construction behindpkg/consolehelpers so theme/accessibility defaults are applied consistently. It removes repeated.WithTheme(styles.HuhTheme).WithAccessible(console.IsAccessibleMode())chains acrosspkg/cli.What changed
pkg/console:PromptFormPromptInputPromptSelectPromptMultiSelectPromptTextPromptConfirm(for parity across confirm prompts)WithTheme(styles.HuhTheme)WithAccessible(console.IsAccessibleMode())CLI migration
pkg/clito use console wrappers instead of localhuh.NewForm(...).WithTheme(...).WithAccessible(...)chains.Console alignment
pkg/consoleprompt implementations (confirm,input,list) to use the same wrappers, ensuring one construction path for themed/accessible forms.Focused coverage
pkg/console/prompt_form_test.gofor wrapper construction behavior.