diff --git a/.ai/skills/new-command.md b/.ai/skills/new-command.md index d2b897b..967f7e8 100644 --- a/.ai/skills/new-command.md +++ b/.ai/skills/new-command.md @@ -162,19 +162,63 @@ Use `cmdutil.IsPromptInterrupt(err)` (Ctrl+C) and `cmdutil.IsPromptBack(err)` (Esc) when the two must differ — e.g. a "Back to list / Exit" gate, or a wizard where Esc steps back one prompt while Ctrl+C exits the whole flow. -**Multi-step wizards.** Model each prompt as its own step walked by an index -into a steps slice: Esc decrements the index (–1 on the first step ends the -flow = exit), Ctrl+C exits, success advances. Print a `Step N of M · Title` -header and a one-time intro banner so the user knows the plan. See -`cmd/s3/move_wizard.go` (`runMoveWizard` + `classifyNav`/`navIdx`) for the -reference pattern. - -> **Pitfall that breaks Esc=back:** a shared picker that swallows cancellation -> into `("", nil)` (i.e. `if IsPromptCancel(err) { return "", nil }`) is fine for -> a top-level command, but inside a wizard it destroys back-navigation — the -> wizard can no longer tell Esc (go back) from Ctrl+C (exit) and Esc ends up -> exiting the whole flow. Wizard-facing pickers MUST return the **raw** prompter -> error so the step loop can classify it. +**Multi-step wizards — ALWAYS use the shared wizard engine.** Do NOT hand-roll a +step loop. Every multi-step interactive flow goes through +`github.com/verda-cloud/verdagostack/pkg/tui/wizard` so they all share one look +(progress bar + hint bar), Esc=back, and Ctrl+C handling. Reference flows: +`cmd/s3/wizard.go` (`buildConfigureFlow`), `cmd/s3/move_wizard.go` +(`buildMoveFlow`), `cmd/vm/wizard.go`. + +Shape of a flow: + +```go +engine := wizard.NewEngine(f.Prompter(), f.Status(), + wizard.WithOutput(ioStreams.ErrOut), wizard.WithExitConfirmation()) +if err := engine.Run(ctx, flow); err != nil { + return err // Ctrl+C returns an error here — propagate it, like configure/vm/mv +} +// Final action (save / preview+confirm / execute) happens AFTER Run — the engine +// has no confirm prompt. See finalizeMove / configure's RunE. +``` + +```go +flow := &wizard.Flow{ + Name: "s3-move", + Layout: []wizard.ViewDef{ + {ID: "progress", View: wizard.NewProgressView(wizard.WithProgressPercent())}, + {ID: "hints", View: wizard.NewHintBarView(wizard.WithHintStyle(bubbletea.HintStyle()))}, + }, + Steps: []wizard.Step{ /* ... */ }, +} +``` + +Per-step rules: +- Each `Step` binds to a variable via `Setter`/`Value`/`IsSet`/`Resetter`. `IsSet()==true` (e.g. a `--flag` was passed) makes the engine **skip** the step and propagate `Value()` into the collected map. +- `SelectPrompt` with a `Loader` for dynamic lists. A Loader may read earlier steps from `store.Collected()[""]`; declare `DependsOn: []string{""}` so it re-runs when that input changes (e.g. mv's source-object list depends on the chosen source bucket). +- `Default(collected)` is applied **only for non-required empty values**. A `Required` step re-prompts on empty and never sees the default — so to pre-fill an optional value (e.g. dest key defaults to source key) make the step `Required: false` + `Default`. + +> **Gotcha that bit us twice — conditional steps + `Resetter`:** a step skipped via +> `ShouldSkip` has its `Resetter` **called by the engine**. If that step shares a +> bound variable with another step — the "+ Create new X…" pattern, where a Select +> writes the existing choice and a conditional name step writes a new one into the +> *same* variable — its `Resetter` MUST be a **no-op**, or skipping it (an existing +> choice was picked) clobbers the selection back to the default. The owning Select +> step keeps the real `Resetter`. + +> **"+ Create new…" pattern:** the Select offers a sentinel choice +> (`"\x00new-…"` — a NUL byte can't be a real bucket/profile name, so no collision). +> The Select's `Setter` must **guard against writing the sentinel** into the bound +> variable (`if s != newSentinel { x = s }`); a separate `ShouldSkip`-gated +> `TextInputPrompt` step collects the new name. See `configureStepProfile` + +> `configureStepNewProfileName`, and `moveStepDestBucket` + `moveStepNewDestBucket`. + +**Testing wizards:** drive the flow with a test engine, not the tuitest prompter +mock (the engine builds its own prompt models). Use +`wizard.NewEngine(nil, nil, wizard.WithOutput(io.Discard), wizard.WithTestResults( +wizard.SelectResult(i), wizard.TextResult(s), …))` and assert the bound state +after `engine.Run`. Test the post-`Run` action (save/confirm/execute) separately +with the tuitest prompter. See `TestBuildConfigureFlowHappyPath`, +`TestBuildMoveFlow_CollectsSelections`, `TestFinalizeMove_S3ToS3`. ### 7. Output Conventions diff --git a/internal/verda-cli/cmd/s3/README.md b/internal/verda-cli/cmd/s3/README.md index 66b4fb9..c72a45d 100644 --- a/internal/verda-cli/cmd/s3/README.md +++ b/internal/verda-cli/cmd/s3/README.md @@ -211,7 +211,7 @@ Profiles work across both API and S3 credentials. Create a second profile via: ```bash verda s3 configure --profile staging ``` -Switch with `--profile staging` on any command, or persist it with `verda auth use staging`. +`configure` and `show` take a local `--profile`. The other commands (`ls`/`cp`/`rm`/…) have no local `--profile` flag — select a profile with the global `--auth.profile staging`, `VERDA_PROFILE=staging`, or persist it with `verda auth use staging`. ## Interactive vs Non-Interactive @@ -272,7 +272,7 @@ Business logic: Wizard flow (`configure`): -1. Profile — **pick an existing profile** (each tagged "S3 configured" / "no S3 credentials yet") **or "+ Create new profile…"**. The selection defaults to the active profile (`--profile` / `VERDA_PROFILE` / `verda auth use`). +1. Profile — **pick an existing profile** (each tagged "S3 configured" / "no S3 credentials yet") **or "+ Create new profile…"**. The selection defaults to the active profile (`--auth.profile` / `VERDA_PROFILE` / `verda auth use`). 2. New profile name — only when "Create new" was chosen. 3. S3 access key ID 4. S3 secret access key (password prompt) @@ -281,4 +281,4 @@ Wizard flow (`configure`): Steps are skipped individually when the corresponding flag is already set — so `verda s3 configure --access-key X --endpoint Y` only prompts for the secret and region, and `--profile staging` skips the profile picker entirely (targeting `[staging]`). -> Note: `configure` writes to the profile you pick/name here; it does **not** auto-follow the active profile the way the read commands (`ls`/`cp`/…) do. The picker defaulting to the active profile keeps the two aligned, but if you create credentials for a non-active profile, pass `--profile` to the read commands or `verda auth use ` to switch. +> Note: `configure` writes to the profile you pick/name here; it does **not** auto-follow the active profile the way the read commands (`ls`/`cp`/…) do. The picker defaulting to the active profile keeps the two aligned, but if you create credentials for a non-active profile, select it on the read commands with `--auth.profile ` / `VERDA_PROFILE`, or `verda auth use ` to switch. diff --git a/internal/verda-cli/cmd/s3/configure_test.go b/internal/verda-cli/cmd/s3/configure_test.go index 20462aa..fa85c7a 100644 --- a/internal/verda-cli/cmd/s3/configure_test.go +++ b/internal/verda-cli/cmd/s3/configure_test.go @@ -16,6 +16,7 @@ package s3 import ( "bytes" + "context" "os" "path/filepath" "testing" @@ -25,6 +26,38 @@ import ( cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" ) +// TestConfigureProfilePicker_HonorsCredentialsFile guards the Medium bug: the +// profile picker must list the SAME file the save writes to. With +// --credentials-file pointing at fileB, the picker should show fileB's profiles, +// not those of the default/env file. +func TestConfigureProfilePicker_HonorsCredentialsFile(t *testing.T) { + // No t.Parallel: t.Setenv. + dir := t.TempDir() + fileA := filepath.Join(dir, "default-creds") + fileB := filepath.Join(dir, "explicit-creds") + if err := os.WriteFile(fileA, []byte("[alpha]\nverda_s3_access_key = a\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(fileB, []byte("[beta]\nverda_s3_access_key = b\n"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("VERDA_SHARED_CREDENTIALS_FILE", fileA) + + step := configureStepProfile(&configureOptions{CredentialsFile: fileB}) + choices, err := step.Loader(context.Background(), nil, nil, nil) + if err != nil { + t.Fatalf("loader: %v", err) + } + var hasAlpha, hasBeta bool + for _, c := range choices { + hasAlpha = hasAlpha || c.Value == "alpha" + hasBeta = hasBeta || c.Value == "beta" + } + if !hasBeta || hasAlpha { + t.Errorf("choices = %+v; want beta (from --credentials-file) and not alpha", choices) + } +} + // TestConfigureFlagMode_DefaultsEndpointAndRegion verifies that supplying only // the keys runs non-interactively and fills in the default endpoint + region. func TestConfigureFlagMode_DefaultsEndpointAndRegion(t *testing.T) { diff --git a/internal/verda-cli/cmd/s3/move_wizard.go b/internal/verda-cli/cmd/s3/move_wizard.go index 7d97154..8a0d3a4 100644 --- a/internal/verda-cli/cmd/s3/move_wizard.go +++ b/internal/verda-cli/cmd/s3/move_wizard.go @@ -192,8 +192,9 @@ func moveStepNewDestBucket(st *moveWizardState) wizard.Step { return nil }, Setter: func(v any) { st.newDstBucket = strings.TrimSpace(v.(string)) }, - // No-op: skipping (an existing dest bucket was chosen) must not clobber state. - Resetter: func() {}, + // Clear on skip: picking an existing dest bucket must drop a stale name, + // else it overrides the chosen bucket in finalizeMove. + Resetter: func() { st.newDstBucket = "" }, IsSet: func() bool { return false }, Value: func() any { return st.newDstBucket }, } diff --git a/internal/verda-cli/cmd/s3/tui_interactive_test.go b/internal/verda-cli/cmd/s3/tui_interactive_test.go index 291801a..7639837 100644 --- a/internal/verda-cli/cmd/s3/tui_interactive_test.go +++ b/internal/verda-cli/cmd/s3/tui_interactive_test.go @@ -120,11 +120,17 @@ func TestRmBrowser_DrillInMultiDelete(t *testing.T) { type mvWizardFake struct { API - buckets []string - objects []string - copiedSrc string - copiedDst string - deleted string + buckets []string + objects []string + copiedSrc string + copiedDst string + deleted string + createdBucket string +} + +func (m *mvWizardFake) CreateBucket(ctx context.Context, in *s3.CreateBucketInput, opts ...func(*s3.Options)) (*s3.CreateBucketOutput, error) { + m.createdBucket = aws.ToString(in.Bucket) + return &s3.CreateBucketOutput{}, nil } func (m *mvWizardFake) ListBuckets(ctx context.Context, in *s3.ListBucketsInput, opts ...func(*s3.Options)) (*s3.ListBucketsOutput, error) { @@ -207,6 +213,76 @@ func TestFinalizeMove_S3ToS3(t *testing.T) { } } +// TestMoveStepNewDestBucket_ResetterClears guards the High bug: when the +// new-bucket step is skipped (an existing dest bucket was picked, possibly after +// backing out of "create new"), its Resetter must drop the typed name, or +// finalizeMove would create + move to the wrong bucket. +func TestMoveStepNewDestBucket_ResetterClears(t *testing.T) { + t.Parallel() + st := &moveWizardState{dstBucket: "dst", newDstBucket: "typed-but-abandoned"} + moveStepNewDestBucket(st).Resetter() + if st.newDstBucket != "" { + t.Errorf("newDstBucket = %q, want empty after skip", st.newDstBucket) + } +} + +// TestBuildMoveFlow_BackOutOfCreateBucket exercises the same bug end-to-end: +// choose "+ Create new bucket", type a name, Esc back to the bucket step, then +// pick an existing bucket. The final state must target the existing bucket with +// no stale new-bucket name. +func TestBuildMoveFlow_BackOutOfCreateBucket(t *testing.T) { + // no t.Parallel — prompter/fake state + fake := &mvWizardFake{buckets: []string{"src", "dst"}, objects: []string{"a.txt"}} + f := cmdutil.NewTestFactory(tuitest.New()) + st := &moveWizardState{} + + engine := wizard.NewEngine(nil, nil, + wizard.WithOutput(io.Discard), + wizard.WithTestResults( + wizard.SelectResult(0), // source bucket: src + wizard.SelectResult(0), // source object: a.txt + wizard.SelectResult(2), // dest bucket: + Create new (idx 2 of src,dst,+create) + wizard.TextResult("new-a"), // new bucket name + wizard.BackResult(), // dest key -> back to new-bucket + wizard.BackResult(), // new-bucket -> back to dest bucket + wizard.SelectResult(1), // dest bucket: dst (existing) + wizard.TextResult("renamed.txt"), // dest key + ), + ) + if err := engine.Run(context.Background(), buildMoveFlow(f, fake, st)); err != nil { + t.Fatalf("engine Run: %v", err) + } + if st.dstBucket != "dst" || st.newDstBucket != "" { + t.Errorf("state = {dstBucket:%q newDstBucket:%q}, want dst / empty", st.dstBucket, st.newDstBucket) + } +} + +// TestFinalizeMove_CreatesNewBucket covers the create-new destination path: +// confirm → CreateBucket → CopyObject into it → delete source. +func TestFinalizeMove_CreatesNewBucket(t *testing.T) { + // no t.Parallel — clientBuilder/prompter state + fake := &mvWizardFake{} + restore := withFakeClient(fake) + defer restore() + + f := cmdutil.NewTestFactory(tuitest.New().AddConfirm(true)) + cmd := NewCmdMv(f, ioBufs()) + st := &moveWizardState{srcBucket: "src", srcKey: "a.txt", newDstBucket: "fresh", dstKey: "a.txt"} + + if err := finalizeMove(context.Background(), cmd, f, ioBufs(), fake, &cpOptions{}, st); err != nil { + t.Fatalf("finalizeMove: %v", err) + } + if fake.createdBucket != "fresh" { + t.Errorf("created bucket = %q, want fresh", fake.createdBucket) + } + if fake.copiedDst != "fresh/a.txt" { + t.Errorf("copy dest = %q, want fresh/a.txt", fake.copiedDst) + } + if fake.deleted != "src/a.txt" { + t.Errorf("source deleted = %q, want src/a.txt", fake.deleted) + } +} + func ioBufs() cmdutil.IOStreams { return cmdutil.IOStreams{Out: &bytes.Buffer{}, ErrOut: &bytes.Buffer{}} } diff --git a/internal/verda-cli/cmd/s3/wizard.go b/internal/verda-cli/cmd/s3/wizard.go index 7399fe1..ebaf665 100644 --- a/internal/verda-cli/cmd/s3/wizard.go +++ b/internal/verda-cli/cmd/s3/wizard.go @@ -82,7 +82,8 @@ func configureStepProfile(opts *configureOptions) wizard.Step { Prompt: wizard.SelectPrompt, Required: true, Loader: func(_ context.Context, _ tui.Prompter, _ tui.Status, _ *wizard.Store) ([]wizard.Choice, error) { - path, err := resolveCredentialsFile("") + // List the same file the save writes to (must honor --credentials-file). + path, err := resolveCredentialsFile(opts.CredentialsFile) if err != nil { return nil, err }