Skip to content

refactor: migrate interactive prompts from survey to huh - #956

Closed
nf-matt wants to merge 26 commits into
fwdcloudsec:mainfrom
nf-matt:huh-prompts
Closed

refactor: migrate interactive prompts from survey to huh#956
nf-matt wants to merge 26 commits into
fwdcloudsec:mainfrom
nf-matt:huh-prompts

Conversation

@nf-matt

@nf-matt nf-matt commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Implements the migration proposed in #945.

AlecAivazis/survey is archived (last release June 2023, 61 open issues) and its API blocks features like ESC-to-cancel. This branch replaces it with huh (actively maintained, generically-typed Select, first-class theming, built-in ESC-cancel) behind the existing pkg/testable shim, and drops the survey dependency.

Why one branch instead of the per-package PRs from the RFC: I didn't hear back on #945, so I've consolidated the work here. The commits still follow the RFC's incremental plan — each is behavior-equivalent, self-contained, and independently revertible, so this can be reviewed commit-by-commit.

Commit progression:

  1. Close the testable-shim leak (callers bypassing the shim).
  2. Add huh v2; introduce a Prompter interface with a huh-backed Confirm; migrate cfaws/env.go.
  3. Add Select / Input / Password methods and migrate their callers, package by package.
  4. Add validator-accepting Input/Select and migrate validator-using callers.
  5. Add SelectWithFilter (bubbles/list) and migrate the assume profile picker — removing the SelectQuestionTemplate monkey-patch and profileNameMap workaround called out in the RFC.
  6. Add ConfirmWithHelp/InputWithHelp and migrate Help-using callers.
  7. Drop the survey dependency.
  8. Theme the huh forms and the filter picker.
  9. Add unit tests for the newly introduced logic (see below).

Tests: the last two commits add coverage for the pure logic this migration introduced in pkg/testable, without requiring a terminal:

  • Required, testInputAsBool, and testInputAsString (the test-mode input helpers).
  • rankFilter (the generic filter ranking).
  • selectFilterModel.Update — the filter-picker key handling, including the ESC-clears-filter-then-cancels behavior that motivated moving off survey. A small no-op refactor (refactor(testable): extract rankFilter and newSelectFilterModel) exposes these seams for testing without changing behavior.

Verification: go build ./... clean; go vet ./pkg/testable/ clean; go test ./... passes with no failures.

nf-matt added 20 commits May 22, 2026 14:52
Adds testable.Ask for multi-question forms and replaces ten direct
survey.AskOne / survey.Ask callers that bypassed the shim.
Adds the Prompter interface and a testable.Confirm function as the
entry point for migrated callers. The huh implementation writes to
os.Stderr (granted's stdout is shell-evaluated) and binds ESC alongside
Ctrl+C to the form's Quit action, since huh v2 binds Ctrl+C only.
Uses generic huh.Select[string] and inherits the ESC-cancel keybinding
from the form keymap.
Replaces twelve survey.Select sites that used only Message and Options.
Each migration drops the survey.WithStdio boilerplate since the
Prompter handles stderr internally. One adjacent Confirm in
browser/detect.go is migrated as well because it shared the
now-removed withStdio variable.
Replaces twelve survey.Input sites. The settings/set.go Int case keeps
the existing string-through-interface{} path (no behaviour change).
Migrates the secret-access-key Password prompt and six Confirm prompts
(alias install, default-browser-firefox, uninstall config, credentials
removal, settings/set Bool case, registry setup). Drops the
var prompt survey.Prompt declaration in settings/set.go since none of
the cases reference it anymore.
Adds InputWithValidator and SelectWithValidator to the Prompter
interface, plus a Required helper for the common non-empty case.
- credentials.go AddCredentialsCommand: survey.MinLength(1) becomes
  testable.Required (equivalent semantics).
- credentials.go ImportCredentialsCommand: custom validator now
  receives the selected string directly instead of a core.OptionAnswer.
- config_yaml.go required-keys prompt: the survey.Ask multi-question
  form only ever held a single Question (var questions was declared
  inside the loop). Collapses to InputWithValidator + SaveKey,
  dropping the ansmap intermediary. The now-unused SaveKeys function
  is removed and SaveKey's stale doc comment is updated.
huh.Select's built-in filter is literal substring only with no public
matcher hook; bubbles/list exposes a Filter field that takes a func,
so the implementation drops one layer down to build the picker
directly.

The non-modal key handler mirrors survey's UX: typing appends to the
filter, arrows navigate, Enter selects, Esc clears the filter or
quits. The default bubbles/list keymap is replaced with bindings that
match what's actually wired up.
QueryProfiles drops the survey.Select call and the associated
SelectQuestionTemplate global-mutation hack used to render the
Profile/Description column header. The header is now pre-printed via
fmt.Fprintln before launching the picker, which works because
bubbles/list runs inline.

filterMultiToken is restored with a simpler (term, opt) signature.
Help-text variants backed by huh's Description() on the underlying
field. Each shares a single private helper with its plain counterpart.
- browser/detect.go SSOBrowser: Confirm with Help -> ConfirmWithHelp
- settings/requesturl/set.go: Input with Help -> InputWithHelp
Removes testable.AskOne and testable.Ask along with the survey and
survey/core imports from pkg/testable. go mod tidy drops
AlecAivazis/survey/v2 from go.mod entirely. The library was archived
by its author in 2024.
Adds grantedTheme (based on huh.ThemeCharm) and styling helpers for
the bubbles/list-based filter picker. Replaces the fuchsia/indigo
accents with ANSI cyan and green, removes the outer left-border around
focused fields, and switches every colour to ANSI 16 indices so the
prompts pick up the user's terminal palette instead of Charm's hex
codes.

The filter picker's focused row indicator becomes a > cursor in cyan
with green option text, matching huh.Select's SelectSelector +
SelectedOption convention. Status bar stays visible to show the active
filter text.
Pull the pure filter-ranking logic and the bubbles/list model construction
out of SelectWithFilter so both can be exercised without opening a terminal.
No behavior change: SelectWithFilter builds the same model and ranks options
identically.
Add unit tests for the logic the huh migration introduced:

- Required, testInputAsBool (bool/parseable-string/parse-error/wrong-type),
  and testInputAsString (string/nil/non-string formatting).
- rankFilter's empty-term, matches, and no-match cases.
- selectFilterModel.Update: enter selects, ctrl+c cancels, printable chars
  build the filter, backspace shortens then clears it, and esc clears an
  active filter before cancelling -- the ESC behavior this migration set out
  to add over survey.
@nf-matt
nf-matt marked this pull request as ready for review July 14, 2026 15:34

@meyerjrr meyerjrr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Code looks good, only some small nit picks on some unneeded code and a rename.

I'm a fan of charmbracelet and their work as well, really awesome to see it used in here.
There were a couple of usability quirks that I'm curious if can be confgured in huh.

  • When using a menu like granted settings set the filter option after initiating with / doesn't display the text being filtered on. Not a huge issue but would be slightly nicer to see what I'm filtering against.
  • Another one (honestly not blocking) but when running some commands we display warnings or info messages before the primary CTA, one example is the registry warning commands. Because huh pushes the CTA to the top of the terminal - these info messages can get cut off, see example image (that is the very top of my terminal):
Image

Before:
Image

Comment thread pkg/testable/prompter.go

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the most part the prompter interface is just a indirection to the huh library - we could probably just use each of these methods directly and drop this interface

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, dropped it. It went in as scaffolding on the assumption that the survey and huh paths would need to coexist while callers moved over; that never happened, huhPrompter was the only implementation, and the migration ran through the package-level functions instead. It wasn't the test seam either — that's the isTesting branch inherited from the old testable.AskOne.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite what I had in mind, the interface is gone but there are still many instances of passthrough functions bloating the code that we could use without, eg:

func Select(message string, options []string) (string, error) {
	return defaultPrompter.Select(message, options)
}

I understand that this was probably following what already existed in the usage of the testable package but I think we take a different approach here.

The wrapper in prompter is trying to replicate what survey does and it just doesn't compose as well and we are bloating the package with many of the same call signatures to solve it.

But I propose here that we (for now) remove most of what is in this file package, and replace it with an export for the huh form that will be used around the granted app so that things stay consistent:

 // form wraps a single huh field with granted's output, keymap and theme.
 // Output goes to stderr: stdout carries the GrantedAssume line the shell evals.
 func form(field huh.Field) *huh.Form {
 	return huh.NewForm(huh.NewGroup(field)).
 		WithOutput(os.Stderr).
 		WithKeyMap(huhKeyMap).
 		WithTheme(huhThemeOpt)
 }

call sights then use the huh API directly:

var confirm bool
err := prompt.Form(huh.NewConfirm().Title("Use Firefox as default Granted browser?").Value(&confirm)).Run()

This has a couple advantages imo but I am happy to hear any pushback from yourself or any maintainers:

  • Nobody has to learn a new granted-specific prompt API they read huh's docs to add.
  • Confirm/ConfirmWithHelp/Select/SelectWithValidator/Input/InputWithValidator/
    InputWithHelp/Password all go, along with confirmWith/inputWith/selectWith and the
    huhPrompter type and defaultPrompter global. The package becomes form(), huhKeyMap,
    grantedTheme, NonEmpty.
  • The stdout requirement for assume stays enforced in exactly one place, which is the only reason not to call huh.NewForm at each site directly.

Comment thread pkg/testable/prompter.go Outdated
}

// Required is a validator that rejects empty input.
var Required = func(s string) error {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var Required = func(s string) error {
var NonEmpty = func(s string) error {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted and incorporated.

nf-matt added 3 commits August 1, 2026 23:20
huhPrompter was the only implementation the migration ever had, and the
package-level prompt functions were what callers actually moved onto, so
the interface was pure indirection. Test mode runs off the isTesting
branch inside each method rather than an injected implementation, so it
was not serving as a seam either.

Keeps the stderr rationale by moving it onto newHuhPrompter, which is
where the interface doc comment had been carrying it.
Taking the review suggestion. Required is already spoken for in this
codebase — it's the urfave/cli flag field, and it shows up that way in
add.go, completion.go and credentials.go — so a validator by the same
name meant the word read two different ways depending on the line you
were on. NonEmpty just says what it checks.
huh.Select on its own is wrong for these menus in two ways, both found in
review.

ESC cannot be made to behave. huh.Select binds it to its filter and we bind
it to the form's Quit, and Form.Update matches Quit before the field sees
the key, so ESC always cancelled -- even mid filter, where the help line
advertised it as a filter control. No keymap fixes that; the precedence is
structural.

Filtering also hides matches. huh carries the pre-filter cursor into the
filtered list and leaves the viewport scrolled past the earlier matches, so
a search matching four options can show one. That is charmbracelet/huh#669,
open since July, fixed upstream in charmbracelet/huh#804 but not released.
The broken state is unexported, so it cannot be repaired from here either.

So drive the field directly instead of through a Form, and rebuild it from
scratch whenever the filter changes. A fresh field starts at the first match
with an unscrolled viewport, which sidesteps the bug rather than repairing
it, and owning the key loop puts ESC back within reach: it drops an active
filter first and cancels only when there is none.

Everything else follows from typing being the filter, as it was under
survey and still is in the profile picker:

- No letter is bound to navigation. huh binds j/k to move and g/G to jump by
  default; those are unbound so they reach the filter, which is the same
  trade survey made explicitly.
- The multi-token filter that was private to assume becomes the default for
  every select, so "prod eu" narrows anywhere. SelectWithFilter goes away;
  nothing needs a custom filter now.
- Selections can be validated. A rejected choice reports why and leaves the
  prompt open rather than losing the command to one bad pick.
- Height only ever shrinks: long lists cap at ten options so warnings printed
  before the prompt stay on screen, and short lists render as they did.
@nf-matt

nf-matt commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing! Your catches deserved thorough answers, and they got me digging into a few things — including the muscle memory a vim-style / filter would break, and a couple of issues in huh itself. The goal was to make things better overall, not just different.

Summary below, since the select code changed shape as a result.

The two quirks you spotted turned out to be one bug, and it's upstream in huh.
huh.Select has a bug where a row that scrolls off the top is the one huh draws the filter text on — which is why the text you were typing wasn't visible. That's charmbracelet/huh#669. I've put a fix up as charmbracelet/huh#804, but it hasn't been merged yet, and the state involved is unexported so it can't be worked around from the outside.

ESC had a related problem I hit while testing that. huh.Select binds it to its filter and I also bound it to the form's Quit, and Form.Update matches Quit before the field ever sees the key. So selects now drive the field directly rather than through a huh.Form, and rebuild it whenever the filter changes. A fresh field starts at the first match with an unscrolled viewport, which sidesteps #669 rather than trying to repair it, putting ESC back within reach: it clears an active filter first and cancels only when there is none.

Two behavioral changes from what you already reviewed:

  • Typing filters directly, with no / first. That's what Survey did (select.go appends any typed key to the filter, and explicitly disables vim mode to do it), and what the assume profile picker has always done. Adopting huh's / would have been a silent change to the most-used command, so instead, no letter is bound to navigation — huh binds j/k and g/G by default and those are unbound here so they reach the filter.
  • Menus cap at ten options. Without a cap the list grows to fill the terminal, which is what pushed the registry warnings off the top of your screen. The height only ever shrinks, so short prompts render exactly as before.

Separately, I noticed settings set and sso-tokens clear list their menus in a different order on every run — they range over a map. That's pre-existing and unrelated to this PR, so I'll send it as its own small change rather than fold it in here.

Happy to split any of this out if you'd rather review the migration on its own.

nf-matt added 3 commits August 2, 2026 01:37
Conflicts were the doc URL move to docs.granted.dev landing on the same
lines as the survey prompts this branch replaces, plus the dependency bumps
overlapping the x/term promotion. Kept upstream's URLs and versions with
this branch's prompts.
Every exported prompt claimed it consumed a value from the stream set by
WithNextSurveyInputFunc. That is true, but nothing calls it, so the docs
led with machinery no caller can see. SelectWithValidator also still
described re-prompting; the picker stays open and reports why instead.
The only conflict was in tokens.go, where fwdcloudsec#963's sort.Strings(tokenList)
landed on the same lines as the survey.Select this branch replaces with
testable.Select. The two changes are orthogonal -- one orders the data,
the other renders the menu -- so kept both: upstream's sort, this branch's
prompt. The settings half of fwdcloudsec#963 merged cleanly.
@nf-matt

nf-matt commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

I fixed the merge conflict.

@meyerjrr meyerjrr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @nf-matt apologies for the delay in review. Haven't a a lot of time recently!

Again very appreciative for the effort to move granted over to a modern selection library. I have a few more requests in how we are shaping the new survey API. The reason is that this code sits right on the path every assume invocation takes, and it touches many commands, so the shape we land on is one we'll live with for a while and is quite important to the CLI's health and code maintainability.

If you have any pushback I am open to feedback, but my feel here is that we do not want to be maintaining much custom cli logic here and instead would rather lean on existing packages an API's to help granted run!

Comment thread pkg/testable/prompter.go
Comment on lines +27 to +28
// huh v2's default Quit binding is ctrl+c only; extend it so ESC also
// cancels the prompt, which matches what users expect from modal pickers.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want this? ctrl+c is pretty standard for cli applications (granted included). Sorry if its explained elsewhere but esc for quit would be undesirable I think

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This a quick picker style UI (not unlike the popular fzf). Longer running TUIs tend to use q or F10 or Ctrl+c or vim-style commands. In those tools, it makes sense because you tend to remain in the UI for an extended time. In shorter-running tools focused around a picker, Esc feels more natural, in my opinion.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to have to disagree here, most cli tools default to ctrl+c for exiting out of a TUI. esc is more often used as a 'go back' in my opinion but rarely a quit

Comment thread pkg/testable/prompter.go

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite what I had in mind, the interface is gone but there are still many instances of passthrough functions bloating the code that we could use without, eg:

func Select(message string, options []string) (string, error) {
	return defaultPrompter.Select(message, options)
}

I understand that this was probably following what already existed in the usage of the testable package but I think we take a different approach here.

The wrapper in prompter is trying to replicate what survey does and it just doesn't compose as well and we are bloating the package with many of the same call signatures to solve it.

But I propose here that we (for now) remove most of what is in this file package, and replace it with an export for the huh form that will be used around the granted app so that things stay consistent:

 // form wraps a single huh field with granted's output, keymap and theme.
 // Output goes to stderr: stdout carries the GrantedAssume line the shell evals.
 func form(field huh.Field) *huh.Form {
 	return huh.NewForm(huh.NewGroup(field)).
 		WithOutput(os.Stderr).
 		WithKeyMap(huhKeyMap).
 		WithTheme(huhThemeOpt)
 }

call sights then use the huh API directly:

var confirm bool
err := prompt.Form(huh.NewConfirm().Title("Use Firefox as default Granted browser?").Value(&confirm)).Run()

This has a couple advantages imo but I am happy to hear any pushback from yourself or any maintainers:

  • Nobody has to learn a new granted-specific prompt API they read huh's docs to add.
  • Confirm/ConfirmWithHelp/Select/SelectWithValidator/Input/InputWithValidator/
    InputWithHelp/Password all go, along with confirmWith/inputWith/selectWith and the
    huhPrompter type and defaultPrompter global. The package becomes form(), huhKeyMap,
    grantedTheme, NonEmpty.
  • The stdout requirement for assume stays enforced in exactly one place, which is the only reason not to call huh.NewForm at each site directly.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again not sure if we want to be supporting a custom selector toolchain inside of granted. The default / and vim motions for navigation are fine UX and most users might even prefer this.

Again might be keen to grab thoughts from another maintainer here @chrnorm @JoshuaWilkes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there were a bunch of other command keys involved, / would make more sense to me. But the UI here is focused on selecting from lists, without other commands or features to jump to. I originally had / but chose to implement it the way your UI originally worked because it just felt more direct and intuitive to me.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I totally get where you are coming from by keeping it how it was before. At the end of the day that was just how the library we picked at the time worked.

I am happy with the updated UX that the huh library brings and would rather not include extra custom logic to keep it in parity with how assume worked previously.

@nf-matt

nf-matt commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

@meyerjrr I've spent quite a bit of time on this effort, and I like where my fork landed. At this stage I think I'll stick with it and just bring over any upstream changes. Thanks for considering the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants