fix(s3): mv create-bucket override + configure picker credentials-file (Codex findings)

.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()["<name>"]`; declare `DependsOn: []string{"<name>"}` 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
 

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 <name>` 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 <name>` / `VERDA_PROFILE`, or `verda auth use <name>` to switch.

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) {

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 },
 	}

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{}}
 }

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
 			}