diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dd9539da2..6e7238cc0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -92,7 +92,7 @@ jobs: - name: Check native macOS picker metadata and profile transactions run: go test ./internal/autocomplete -run '^TestPicker' -count=1 -v - name: Check native macOS shell targeting and setup decisions - run: go test ./pkg/custom -run '^TestImagePicker(Shell|FirstRun|TabChoice|MacOS)' -count=1 -v + run: go test ./internal/commands/images ./pkg/custom -run '^TestImagePicker(Shell|FirstRun|TabChoice|MacOS)' -count=1 -v minimum-unix-shells: name: Minimum Unix executable suggestions @@ -833,7 +833,7 @@ jobs: run: go test ./internal/autocomplete -run '^TestPicker(ProfileRecovery.*|Rename.*|RecoveryDirectory.*|Windows.*|FileMetadataPreservesZoneIdentifierAtSnapshotRecheck|ReplacementMetadataKeepsSameInheritedPermissions|SecurityDescriptorComparisonPreservesPermissions|InstallRoundTrip|InstallConfigDirectoryChanges|InstallIgnoresUnusedConfigDirectory|InstallConfigDirectoryRefusalPreservesFiles|InstallPreservesHardLinkedProfiles|ProfileHardLinkAddedAfterSnapshot|ScriptContentChecksLegacyNamespaces|InstallRetriesSupersededScriptCleanup|RemoveRetriesScriptCleanup|RemoveMissingProfileParentCleansOwnedScripts|RemoveMissingProfileAndScriptDirectoriesNoOp|RemoveMissingProfileParentReappearsWhileWaiting|RemovePreservesPersonalLineBoundaries|InstalledDetectsAndRefreshesOlderScript)$' -count=1 -v - name: Check Windows shell home targeting if: matrix.existing-checks - run: go test ./pkg/custom -run '^TestImagePicker(WindowsShellHome|ShellConsent)' -count=1 -v + run: go test ./internal/commands/images -run '^TestImagePicker(WindowsShellHome|ShellConsent)' -count=1 -v - name: Check native PowerShell executable suggestions shell: pwsh run: | diff --git a/.github/workflows/help-compatibility.yml b/.github/workflows/help-compatibility.yml index 52f7d9073..984f7ff24 100644 --- a/.github/workflows/help-compatibility.yml +++ b/.github/workflows/help-compatibility.yml @@ -533,7 +533,7 @@ jobs: set -euo pipefail go test -p 2 ./internal/imageprefs -count=1 -timeout=3m -v -json -run '^(TestImagePreferenceDefaultAndReplacement|TestImagePreferenceInvalidSettingsRemainUntouched|TestImagePreferenceConcurrentWritesAndCancellation|TestImagePreferenceWaitingWriterKeepsNewSchema|TestImagePreferenceWaitingWriterCancellationAndReplacement|TestImagePreferenceRejectsUnsafePaths|TestImagePreferenceRejectsUnsafeWriterLocks|TestImagePreferencePermissionFailuresKeepFile)$' \ | tee "$RUNNER_TEMP/image-preference-state-tests.jsonl" - go test -p 2 ./pkg/custom -count=1 -timeout=3m -v -json -run '^(TestImagePickerStateRoundTripAndPrivateReplacement|TestImagePickerStateIgnoresLegacyPrompt|TestImagePickerStateMalformedAndUnknownRemainUntouched|TestImagePickerStateCanceledWriteKeepsPreviousAndCleansTemporary|TestImagePickerStateConcurrentSavesKeepWholeRecords|TestImagePickerStateLimitIncludesPromptWithoutLimitingGeneration|TestImagePickerStateAcceptsExactPersistenceLimit|TestImagePickerStateRejectsUnstableSaveFolders|TestImagePickerStateRejectsSpecialFilesAndSymlinks|TestImagePickerStateRejectsExposedPermissions|TestImagePickerStateWriteFailureKeepsPrevious)$' \ + go test -p 2 ./internal/commands/images -count=1 -timeout=3m -v -json -run '^(TestImagePickerStateRoundTripAndPrivateReplacement|TestImagePickerStateIgnoresLegacyPrompt|TestImagePickerStateMalformedAndUnknownRemainUntouched|TestImagePickerStateCanceledWriteKeepsPreviousAndCleansTemporary|TestImagePickerStateConcurrentSavesKeepWholeRecords|TestImagePickerStateLimitIncludesPromptWithoutLimitingGeneration|TestImagePickerStateAcceptsExactPersistenceLimit|TestImagePickerStateRejectsUnstableSaveFolders|TestImagePickerStateRejectsSpecialFilesAndSymlinks|TestImagePickerStateRejectsExposedPermissions|TestImagePickerStateWriteFailureKeepsPrevious)$' \ | tee "$RUNNER_TEMP/image-picker-state-tests.jsonl" - name: Test Windows image preferences and picker state if: matrix.image-state && runner.os == 'Windows' && matrix.shard != 'dispatch' @@ -546,7 +546,7 @@ jobs: go test -p 2 ./internal/imageprefs -count=1 -timeout=3m -v -json -run '^(TestImagePreferenceDefaultAndReplacement|TestImagePreferenceInvalidSettingsRemainUntouched|TestImagePreferenceConcurrentWritesAndCancellation|TestImagePreferenceWaitingWriterKeepsNewSchema|TestImagePreferenceWaitingWriterCancellationAndReplacement|TestImagePreferenceRejectsUnsafePaths|TestImagePreferenceRejectsUnsafeWriterLocks|TestImagePreferenceWindowsAccessDeniedKeepsFile)$' | Tee-Object -FilePath (Join-Path $env:RUNNER_TEMP 'image-preference-state-tests.jsonl') -Encoding utf8NoBOM -ErrorAction Stop if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } - go test -p 2 ./pkg/custom -count=1 -timeout=3m -v -json -run '^(TestImagePickerStateRoundTripAndPrivateReplacement|TestImagePickerStateIgnoresLegacyPrompt|TestImagePickerStateMalformedAndUnknownRemainUntouched|TestImagePickerStateCanceledWriteKeepsPreviousAndCleansTemporary|TestImagePickerStateConcurrentSavesKeepWholeRecords|TestImagePickerStateLimitIncludesPromptWithoutLimitingGeneration|TestImagePickerStateAcceptsExactPersistenceLimit|TestImagePickerStateRejectsUnstableSaveFolders|TestImagePickerStateRejectsSpecialFilesAndSymlinks|TestImagePickerStateWindowsAccessDeniedKeepsPrevious)$' | + go test -p 2 ./internal/commands/images -count=1 -timeout=3m -v -json -run '^(TestImagePickerStateRoundTripAndPrivateReplacement|TestImagePickerStateIgnoresLegacyPrompt|TestImagePickerStateMalformedAndUnknownRemainUntouched|TestImagePickerStateCanceledWriteKeepsPreviousAndCleansTemporary|TestImagePickerStateConcurrentSavesKeepWholeRecords|TestImagePickerStateLimitIncludesPromptWithoutLimitingGeneration|TestImagePickerStateAcceptsExactPersistenceLimit|TestImagePickerStateRejectsUnstableSaveFolders|TestImagePickerStateRejectsSpecialFilesAndSymlinks|TestImagePickerStateWindowsAccessDeniedKeepsPrevious)$' | Tee-Object -FilePath (Join-Path $env:RUNNER_TEMP 'image-picker-state-tests.jsonl') -Encoding utf8NoBOM -ErrorAction Stop if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } - name: Test request configuration @@ -871,7 +871,7 @@ jobs: picker_cases["TestImagePickerStateWriteFailureKeepsPrevious"] = [] for filename, package, cases in [ ("image-preference-state-tests.jsonl", "internal/imageprefs", preference_cases), - ("image-picker-state-tests.jsonl", "pkg/custom", picker_cases), + ("image-picker-state-tests.jsonl", "internal/commands/images", picker_cases), ]: suites[filename] = set().union(*(required_cases(root, children) for root, children in cases.items())) strict_packages[filename] = "github.com/openai/openai-cli/" + package @@ -1720,12 +1720,14 @@ jobs: if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } go test -c -o (Join-Path $env:RUNNER_TEMP 'wsl-app.test') ./internal/app if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + go test -c -o (Join-Path $env:RUNNER_TEMP 'wsl-images.test') ./internal/commands/images + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } go mod verify exit $LASTEXITCODE - name: Run compatibility checks in disposable Ubuntu WSL 1 shell: pwsh timeout-minutes: 16 - run: python -I -B scripts/check-wsl-pager.py --binary (Join-Path $env:RUNNER_TEMP 'wsl-pager.test') --process-binary (Join-Path $env:RUNNER_TEMP 'wsl-process.test') --app-binary (Join-Path $env:RUNNER_TEMP 'wsl-app.test') ${{ matrix.arguments }} + run: python -I -B scripts/check-wsl-pager.py --binary (Join-Path $env:RUNNER_TEMP 'wsl-pager.test') --process-binary (Join-Path $env:RUNNER_TEMP 'wsl-process.test') --app-binary (Join-Path $env:RUNNER_TEMP 'wsl-app.test') --images-binary (Join-Path $env:RUNNER_TEMP 'wsl-images.test') ${{ matrix.arguments }} - name: Reconcile the owned WSL distribution if: always() shell: pwsh diff --git a/AGENTS.md b/AGENTS.md index de4c3fa4a..00a8b5c82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,7 +13,7 @@ it. Fix recurring generated defects in the authoritative schema or generator when available instead of introducing broad manual changes. Handwritten CLI features are moving into `internal/commands//` (Codex, -tokenizer, and manpages are there now); remaining behavior lives in `pkg/custom/`. API response selection +tokenizer, manpages, files, batches, models, admin, audio, and images are there now); remaining behavior lives in `pkg/custom/`. API response selection and reshaping live in `internal/output/transform/`. Use cohesive `internal/` helpers for shared implementation, including text rendering and terminal escaping. `internal/commands/` owns command-tree routing, compatibility aliases, shortcuts, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fc895566c..3683014b3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -57,6 +57,9 @@ below describes the current layout as the migration proceeds. the encoding engine. - `internal/commands/manpages/` owns manual rendering, text/gzip file cleanup, and tests. Application assembly supplies the shared save-receipt policy. +- `internal/commands/images/` owns image model discovery, picker drafts, saving, + streaming previews, local preview preferences, and shell setup mechanics. + Assembly supplies request/presentation policy and foreground first-run admission. - `internal/commands/audio/` owns speech controls, local history/playback commands, voice creation and validation, and transcript/speech response handling. Assembly supplies request and presentation policies. @@ -110,7 +113,7 @@ recurring generated problems in the authoritative schema or generator when available. Handwritten features are moving into `internal/commands//`; Codex, -tokenizer, manpages, files, batches, models, admin, and audio are there now. Remaining behavior belongs in `pkg/custom/`, +tokenizer, manpages, files, batches, models, admin, audio, and images are there now. Remaining behavior belongs in `pkg/custom/`, `internal/output/transform/`, and focused `internal/` helpers. Keep `cmd/openai/` focused on startup and wiring. Generated code calls handwritten request/output helpers through a generated runtime bridge. diff --git a/docs/architecture/repository-layout.md b/docs/architecture/repository-layout.md index af27442de..ffbba0233 100644 --- a/docs/architecture/repository-layout.md +++ b/docs/architecture/repository-layout.md @@ -220,6 +220,19 @@ handling. Shared choice labels live in `internal/readable/`. Native Windows publication and voice validation contracts retain their exact test/subcase inventories and now verify their owning packages. +Image model discovery, picker/editor state, image request preparation, saving, +stream handling, local previews/preferences, and shell setup mechanics live in +`internal/commands/images/`. Assembly supplies request preparation and presentation +callbacks, retains output-route eligibility, and owns first-run worker admission +and the foreground timeout. Feature metadata and saved-image stream cleanup stay +private to the feature. Cross-layer tests use real assembly callbacks; native +state-persistence checks retain their exact test and subcase inventories. + +Delayed loading feedback shared by images and model listing lives in +`internal/output/`. Terminal writer classification lives in `internal/terminal/` +and preserves both file and borrowed-file handling. These shared packages do not +import command features or assembly. + ## Product and compatibility contract This repository ships the `openai` executable. Its Go packages are implementation diff --git a/pkg/custom/display_image_progress.go b/internal/commands/images/display_image_progress.go similarity index 96% rename from pkg/custom/display_image_progress.go rename to internal/commands/images/display_image_progress.go index 6566a7c11..20f9a15dc 100644 --- a/pkg/custom/display_image_progress.go +++ b/internal/commands/images/display_image_progress.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -12,6 +12,7 @@ import ( "github.com/charmbracelet/x/term" clioutput "github.com/openai/openai-cli/internal/output" "github.com/openai/openai-cli/internal/readable" + cliterminal "github.com/openai/openai-cli/internal/terminal" "github.com/openai/openai-cli/internal/terminalimage" "github.com/tidwall/gjson" ) @@ -50,7 +51,7 @@ func (p *imageOutputPlan) displayImageProgress(ctx context.Context, event gjson. // Share the saved-image sizing calculation, but not its recovery messages: // the final image has not arrived yet and there is no output file to retry. file, ok := out.(*os.File) - if !ok || !isTerminal(out) { + if !ok || !cliterminal.IsWriterTerminal(out) { return false, errors.New("inline previews require a terminal") } width, height, sizeErr := term.GetSize(file.Fd()) diff --git a/pkg/custom/display_image_progress_test.go b/internal/commands/images/display_image_progress_test.go similarity index 97% rename from pkg/custom/display_image_progress_test.go rename to internal/commands/images/display_image_progress_test.go index ba53fae36..42b0915c0 100644 --- a/pkg/custom/display_image_progress_test.go +++ b/internal/commands/images/display_image_progress_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" @@ -157,7 +157,7 @@ func TestImageProgressTerminal(t *testing.T) { } ctx, cancel := context.WithCancel(t.Context()) defer cancel() - stream := &progressTestStream{savedImageTestStream: savedImageTestStream{events: []outputJSON{{first}, {first}, {second}, {progressTestEvent(t, "image_generation.completed", 0)}}}} + stream := &progressTestStream{savedImageTestStream: savedImageTestStream{events: []outputJSON{{Result: first}, {Result: first}, {Result: second}, {Result: progressTestEvent(t, "image_generation.completed", 0)}}}} stream.beforeNext = func(index int) { if index == 3 { // TERM_SESSION_ID is empty, so opted-in font attempts stop before @@ -176,7 +176,7 @@ func TestImageProgressTerminal(t *testing.T) { stream.err = errors.New("synthetic-private-stream-error") } fmt.Fprintln(os.Stdout, "PROGRESS-CASE", scenario) - err := saveFinalImageStream(ctx, stream, plan, os.Stdout) + err := saveFinalImageStream(ctx, stream, plan, os.Stdout, false) fmt.Fprintln(os.Stdout, "PROGRESS-END", scenario) if scenario == "cancel" || scenario == "malformed-cancel" { require.ErrorIs(t, err, context.Canceled) diff --git a/pkg/custom/display_saved_image.go b/internal/commands/images/display_saved_image.go similarity index 97% rename from pkg/custom/display_saved_image.go rename to internal/commands/images/display_saved_image.go index 7d4b171e7..cf397ebb8 100644 --- a/pkg/custom/display_saved_image.go +++ b/internal/commands/images/display_saved_image.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -15,6 +15,7 @@ import ( "github.com/openai/openai-cli/internal/imageoutput" clioutput "github.com/openai/openai-cli/internal/output" "github.com/openai/openai-cli/internal/readable" + cliterminal "github.com/openai/openai-cli/internal/terminal" "github.com/openai/openai-cli/internal/terminalimage" ) @@ -44,7 +45,7 @@ func displaySavedImages(ctx context.Context, saved []imageoutput.SavedImage, out // displaySavedImage previews only bytes from the completed save operation. It // reports completed rendering and never opens a viewer or changes the saved file. func displaySavedImage(ctx context.Context, saved imageoutput.SavedImage, out, diagnostics io.Writer, mode string) (bool, error) { - protocol := savedImageProtocol(mode, isTerminal(out), os.Getenv) + protocol := savedImageProtocol(mode, cliterminal.IsWriterTerminal(out), os.Getenv) if protocol == "" { return false, nil } @@ -75,7 +76,7 @@ func displaySavedImage(ctx context.Context, saved imageoutput.SavedImage, out, d // local preview, whose caller reports invalid input before reaching this point. func displayDecodedSavedImage(ctx context.Context, img image.Image, out, diagnostics io.Writer, protocol string) error { file, ok := out.(*os.File) - if !ok || !isTerminal(out) { + if !ok || !cliterminal.IsWriterTerminal(out) { return errors.New("inline previews require a terminal") } if diagnostics == nil { diff --git a/pkg/custom/display_saved_image_test.go b/internal/commands/images/display_saved_image_test.go similarity index 99% rename from pkg/custom/display_saved_image_test.go rename to internal/commands/images/display_saved_image_test.go index 1341e005c..bd56fff2a 100644 --- a/pkg/custom/display_saved_image_test.go +++ b/internal/commands/images/display_saved_image_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" diff --git a/pkg/custom/display_saved_images_test.go b/internal/commands/images/display_saved_images_test.go similarity index 99% rename from pkg/custom/display_saved_images_test.go rename to internal/commands/images/display_saved_images_test.go index 588f25a68..20d05dfde 100644 --- a/pkg/custom/display_saved_images_test.go +++ b/internal/commands/images/display_saved_images_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" diff --git a/internal/commands/images/export_test.go b/internal/commands/images/export_test.go new file mode 100644 index 000000000..803d7f010 --- /dev/null +++ b/internal/commands/images/export_test.go @@ -0,0 +1,68 @@ +package images + +import ( + "context" + "io" + + "github.com/openai/openai-cli/internal/imagemodels" + clioutput "github.com/openai/openai-cli/internal/output" + "github.com/urfave/cli/v3" +) + +// Test-only access to private feature state for direct integration tests. +type TestLoadingTestWriter = loadingTestWriter + +var TestIsTerminal = isTerminal +var TestImagePickerTabChoicePath = imagePickerTabChoicePath +var TestImagePickerSelectShellTargets = imagePickerSelectShellTargets +var TestRegisterImageSavingFlags = registerImageSavingFlags +var TestFirstRunFileSnapshot = firstRunFileSnapshot +var TestImageInlinePreferenceCommands = imageInlinePreferenceCommands +var TestImageSavingWorkflow = imageSavingWorkflow +var TestSetupImagePickerFirstRun = setupImagePickerFirstRun +var TestImageModelTestRow = imageModelTestRow +var TestImageInlinePreferenceLocation = imageInlinePreferenceLocation +var TestImagePickerShellTarget = imagePickerShellTarget + +type TestImageModelsReport = imageModelsReport + +var TestPickerShellSetupHome = pickerShellSetupHome +var TestWriteImageModelsData = writeImageModelsData +var TestImagePickerTabDeclined = imagePickerTabDeclined +var TestImageInlinePreferencePath = imageInlinePreferencePath +var TestLegacyImagePickerTabChoicePath = legacyImagePickerTabChoicePath +var TestDeclineImagePickerTab = declineImagePickerTab +var TestFinishInteractiveImagePreview = finishInteractiveImagePreview +var TestPrivateImagePickerState = privateImagePickerState + +type TestOutputJSON = outputJSON + +var TestImagePickerStatePath = imagePickerStatePath + +func NewOutputPlanForTest(directory, name, inline string) *imageOutputPlan { + return &imageOutputPlan{directory: directory, name: name, inline: inline} +} +func (p *imageOutputPlan) DirectoryForTest() string { return p.directory } +func (p *imageOutputPlan) SaveForTest(ctx context.Context, data []byte, out io.Writer) error { + return p.save(ctx, data, out) +} +func (p *imageOutputPlan) SetLoadingStageForTest(stage func(clioutput.LoadingStage)) { + p.loadingStage = stage +} +func WithPresentationForTest(ctx context.Context, plan *imageOutputPlan, out io.Writer) context.Context { + return context.WithValue(ctx, imagePresentationKey{}, imagePresentation{plan, out}) +} +func InlineForTest(ctx context.Context) string { + return ctx.Value(imagePresentationKey{}).(imagePresentation).plan.inline +} +func PreparedBodyForTest(command *cli.Command) io.ReadCloser { + return command.Metadata[imageMultipartMetadata].(*imageMultipartPreparation).body.(io.ReadCloser) +} +func InterruptForTest(err error) error { return &imageLoadingInterrupt{err} } +func InteractiveExitForTest(code int) error { return interactiveImageExit{code} } +func ModelsErrorForTest(invocation string, extra bool, results []imagemodels.Result) *ModelsError { + return &ModelsError{invocation: invocation, extraArguments: extra, results: results} +} +func ShellPathsForTest(home, config, zdotdir string) imagePickerShellPaths { + return imagePickerShellPaths{home: home, config: config, zdotdir: zdotdir} +} diff --git a/pkg/custom/image_generation_settings.go b/internal/commands/images/image_generation_settings.go similarity index 99% rename from pkg/custom/image_generation_settings.go rename to internal/commands/images/image_generation_settings.go index 5874f21e6..f78b5e93f 100644 --- a/pkg/custom/image_generation_settings.go +++ b/internal/commands/images/image_generation_settings.go @@ -1,4 +1,4 @@ -package custom +package images import ( "errors" diff --git a/pkg/custom/image_generation_settings_test.go b/internal/commands/images/image_generation_settings_test.go similarity index 99% rename from pkg/custom/image_generation_settings_test.go rename to internal/commands/images/image_generation_settings_test.go index f9cffa6dc..60f9d405a 100644 --- a/pkg/custom/image_generation_settings_test.go +++ b/internal/commands/images/image_generation_settings_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "strings" diff --git a/internal/commands/images/image_loading_feedback.go b/internal/commands/images/image_loading_feedback.go new file mode 100644 index 000000000..2c5694903 --- /dev/null +++ b/internal/commands/images/image_loading_feedback.go @@ -0,0 +1,88 @@ +package images + +import ( + "context" + "errors" + "os" + "os/signal" + "runtime" + + "github.com/charmbracelet/x/term" + clioutput "github.com/openai/openai-cli/internal/output" + cliterminal "github.com/openai/openai-cli/internal/terminal" + "github.com/urfave/cli/v3" +) + +// Feedback starts after request preparation, so it never changes stdin reading +// or validation. It owns no request/output writers and cannot replace a failure. +func runWithImageLoading(ctx context.Context, command *cli.Command, plan *imageOutputPlan, next cli.ActionFunc, opts SavingOptions) (err error) { + root := command.Root() + out := imageCommandWriter(command) + if plan == nil || !cliterminal.IsWriterTerminal(out) || + root.Bool("debug") || opts.ErrorFormat(root) != "text" || root.String("transform-error") != "" { + return next(ctx, command) + } + feedback := os.Stderr + if plan.loadingPrompt != "" { + // Keep the visible picker prompt on its UI surface, outside diagnostics. + feedback = out.(*os.File) // isTerminal above requires an *os.File. + } + if !cliterminal.IsWriterTerminal(feedback) { + return next(ctx, command) + } + label := "Generating image" + switch command.Name { + case "edit": + label = "Editing image" + case "create-variation": + label = "Creating image variation" + } + parent := ctx + ctx, stopSignals := signal.NotifyContext(ctx, os.Interrupt) + // Restore normal signal handling once cancellation starts. A second Ctrl-C + // can still terminate the process if a terminal write or cleanup is blocked. + stopReset := context.AfterFunc(ctx, stopSignals) + stopLoading := func() {} + advance := func(clioutput.LoadingStage) {} + if opts.DiagnosticsAllowed(ctx) { + stopLoading, advance = clioutput.StartLoadingFeedback(ctx, feedback, label, plan.loadingPrompt, clioutput.LoadingAnimationSupported(os.Getenv), clioutput.LoadingSpinner(os.Getenv, runtime.GOOS), func() (int, int) { + width, height, _ := term.GetSize(feedback.Fd()) + return width, height + }) + } + plan.stopLoading = stopLoading + plan.loadingStage = advance + defer func() { + stopLoading() + stopReset() + interrupted := ctx.Err() != nil && parent.Err() == nil + stopSignals() + var exit cli.ExitCoder + if interrupted && (err == nil || errors.Is(err, context.Canceled)) && !errors.As(err, &exit) { + if err == nil { + err = ctx.Err() + } + err = &imageLoadingInterrupt{err} + } + }() + return next(ctx, command) +} + +// Keep the cancellation cause and any saving guidance while using the shell's +// conventional Ctrl-C status. Ordinary API/local errors keep their own status. +type imageLoadingInterrupt struct{ error } + +func (e *imageLoadingInterrupt) Unwrap() error { return e.error } +func (e *imageLoadingInterrupt) ExitCode() int { return 130 } + +func (p *imageOutputPlan) stopLoadingFeedback() { + if p.stopLoading != nil { + p.stopLoading() + } +} + +func (p *imageOutputPlan) setLoadingStage(stage clioutput.LoadingStage) { + if p != nil && p.loadingStage != nil { + p.loadingStage(stage) + } +} diff --git a/internal/commands/images/image_loading_feedback_test.go b/internal/commands/images/image_loading_feedback_test.go new file mode 100644 index 000000000..be83b4722 --- /dev/null +++ b/internal/commands/images/image_loading_feedback_test.go @@ -0,0 +1,137 @@ +package images + +import ( + "bytes" + "context" + "errors" + "io" + "sync" + "testing" + "testing/synctest" + "time" + + "charm.land/bubbles/v2/spinner" + clioutput "github.com/openai/openai-cli/internal/output" + "github.com/stretchr/testify/require" + "github.com/tidwall/gjson" + "github.com/urfave/cli/v3" +) + +func TestImageLoadingPromptUsesOnlyCurrentPickerDisplay(t *testing.T) { + const privatePrompt = "SYNTHETIC_INDIRECT_REQUEST_CONTENT" + const visiblePrompt = "Synthetic prompt shown in the picker" + app := &cli.Command{Name: "generate"} + registerImageSavingFlags(app) + app.Action = func(ctx context.Context, command *cli.Command) error { + body := gjson.Parse(`{"prompt":"` + privatePrompt + `","model":"gpt-image-2"}`) + pickerCtx := context.WithValue(ctx, imagePickerLoadingPromptKey{}, visiblePrompt) + for _, test := range []struct { + name string + ctx context.Context + want string + }{ + {"direct request", ctx, ""}, + {"picker request", pickerCtx, visiblePrompt}, + {"reused parent", ctx, ""}, + } { + t.Run(test.name, func(t *testing.T) { + plan, _, err := prepareImageSaving(test.ctx, command, body) + require.NoError(t, err) + require.NotNil(t, plan) + require.Equal(t, privatePrompt, body.Get("prompt").String(), "display selection must not modify request data") + synctest.Test(t, func(t *testing.T) { + var output bytes.Buffer + stop, _ := clioutput.StartLoadingFeedback(t.Context(), &output, "Generating image", plan.loadingPrompt, true, spinner.Line, func() (int, int) { return 100, 24 }) + time.Sleep(clioutput.LoadingDelay + time.Second) + stop() + require.NotContains(t, output.String(), privatePrompt) + require.Contains(t, output.String(), "Generating image") + require.Contains(t, output.String(), "1s elapsed") + if test.want == "" { + require.NotContains(t, output.String(), visiblePrompt) + require.NotContains(t, output.String(), "Generating image '") + } else { + require.Contains(t, output.String(), "Generating image '"+test.want+"'") + } + }) + }) + } + return nil + } + require.NoError(t, app.Run(t.Context(), []string{"openai", "--output-dir", t.TempDir(), "--name", "synthetic-safe-name"})) +} + +type lockedLoadingBuffer struct { + mu sync.Mutex + buffer bytes.Buffer +} + +func (b *lockedLoadingBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buffer.Write(p) +} + +func (b *lockedLoadingBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buffer.String() +} + +func TestImageLoadingInterruptRetainsCause(t *testing.T) { + cause := imageSavingFailure("Check saved files before trying again.", context.Canceled) + err := &imageLoadingInterrupt{cause} + require.ErrorIs(t, err, context.Canceled) + var exit cli.ExitCoder + require.ErrorAs(t, err, &exit) + require.Equal(t, 130, exit.ExitCode()) + var saving *SavingError + require.ErrorAs(t, err, &saving) + require.Equal(t, cause.Error(), err.Error()) +} + +func TestImageLoadingStopsBeforeVisibleOutput(t *testing.T) { + for _, progress := range []bool{false, true} { + stopped, wrote := false, false + plan := &imageOutputPlan{ + directory: t.TempDir(), name: "synthetic", inline: "off", + stopLoading: func() { stopped = true }, + } + writer := loadingTestWriter(func(p []byte) (int, error) { + require.True(t, stopped, "feedback must stop before any image label, warning or saved path") + wrote = true + return len(p), nil + }) + if progress { + _, err := plan.displayImageProgress(t.Context(), gjson.Parse(`{}`), writer, "kitty") + require.NoError(t, err) + } else { + require.NoError(t, plan.save(t.Context(), []byte(`{"data":[{"b64_json":"iVBORw0KGgo="}]}`), writer)) + } + require.True(t, wrote) + } +} + +type loadingTestWriter func([]byte) (int, error) + +func (w loadingTestWriter) Write(p []byte) (int, error) { return w(p) } + +func TestImageLoadingLeavesNoninteractiveActionUnchanged(t *testing.T) { + for _, plan := range []*imageOutputPlan{nil, {}} { + command := &cli.Command{Name: "generate", Writer: io.Discard} + ctx := t.Context() + sentinel := errors.New("original action failure") + called := false + err := runWithImageLoading(ctx, command, plan, func(got context.Context, gotCommand *cli.Command) error { + called = true + require.Same(t, ctx, got) + require.Same(t, command, gotCommand) + return sentinel + }, SavingOptions{}) + require.True(t, called) + require.Same(t, sentinel, err) + if plan != nil { + require.Nil(t, plan.stopLoading) + } + } +} diff --git a/pkg/custom/image_loading_stages_test.go b/internal/commands/images/image_loading_stages_test.go similarity index 79% rename from pkg/custom/image_loading_stages_test.go rename to internal/commands/images/image_loading_stages_test.go index ec2b17888..51062c535 100644 --- a/pkg/custom/image_loading_stages_test.go +++ b/internal/commands/images/image_loading_stages_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" @@ -9,6 +9,7 @@ import ( "path/filepath" "testing" + clioutput "github.com/openai/openai-cli/internal/output" "github.com/stretchr/testify/require" ) @@ -36,13 +37,13 @@ func TestImageLoadingSaveStagesFollowRetainedFiles(t *testing.T) { cancel() } folder := t.TempDir() - var stages []imageLoadingStage + var stages []clioutput.LoadingStage stopped := false plan := &imageOutputPlan{directory: folder, name: "synthetic", inline: "none"} - plan.loadingStage = func(stage imageLoadingStage) { + plan.loadingStage = func(stage clioutput.LoadingStage) { require.False(t, stopped) stages = append(stages, stage) - if stage == imageLoadingSaved { + if stage == clioutput.LoadingSaved { files, err := filepath.Glob(filepath.Join(folder, "*.png")) require.NoError(t, err) require.Len(t, files, test.count) @@ -58,10 +59,10 @@ func TestImageLoadingSaveStagesFollowRetainedFiles(t *testing.T) { err := plan.save(ctx, []byte(test.response), &output) if test.wantErr { require.Error(t, err) - require.Equal(t, []imageLoadingStage{imageLoadingSaving}, stages) + require.Equal(t, []clioutput.LoadingStage{clioutput.LoadingSaving}, stages) } else { require.NoError(t, err) - require.Equal(t, []imageLoadingStage{imageLoadingSaving, imageLoadingSaved}, stages) + require.Equal(t, []clioutput.LoadingStage{clioutput.LoadingSaving, clioutput.LoadingSaved}, stages) } require.True(t, stopped) files, err := filepath.Glob(filepath.Join(folder, "*.png")) @@ -73,6 +74,6 @@ func TestImageLoadingSaveStagesFollowRetainedFiles(t *testing.T) { func TestImageLoadingStageWithoutFeedbackIsSafe(t *testing.T) { var absent *imageOutputPlan - absent.setLoadingStage(imageLoadingSaving) - (&imageOutputPlan{}).setLoadingStage(imageLoadingSaved) + absent.setLoadingStage(clioutput.LoadingSaving) + (&imageOutputPlan{}).setLoadingStage(clioutput.LoadingSaved) } diff --git a/pkg/custom/image_models.go b/internal/commands/images/image_models.go similarity index 85% rename from pkg/custom/image_models.go rename to internal/commands/images/image_models.go index 20ab61e58..38b75ff79 100644 --- a/pkg/custom/image_models.go +++ b/internal/commands/images/image_models.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -14,6 +14,8 @@ import ( "github.com/openai/openai-cli/internal/apiquery" "github.com/openai/openai-cli/internal/imagemodels" "github.com/openai/openai-cli/internal/readable" + requestconfig "github.com/openai/openai-cli/internal/request" + cliterminal "github.com/openai/openai-cli/internal/terminal" "github.com/openai/openai-go/v3" "github.com/tidwall/gjson" "github.com/urfave/cli/v3" @@ -39,9 +41,9 @@ Readable output also works in scripts. Failed checks keep partial results and ex API key setup: {{$bin}} help setup ` -// Image-model discovery is a CLI workflow over the existing model retrieval +// RegisterModels installs image model discovery, a CLI workflow over the existing model retrieval // operation. Keep generated /models listing and its script contract untouched. -func registerImageModels(root *cli.Command) { +func RegisterModels(root *cli.Command, opts ModelOptions) { for _, resource := range root.Commands { if resource.Name == "images" { resource.Commands = append(resource.Commands, &cli.Command{ @@ -52,7 +54,7 @@ func registerImageModels(root *cli.Command) { &cli.BoolFlag{Name: "all", Usage: "Include known snapshots, retired models and models not visible to your key", HideDefault: true}, &cli.BoolFlag{Name: "offline", Usage: "Show known names without an API request or access check", HideDefault: true}, }, - Action: handleImagesModels, + Action: func(ctx context.Context, command *cli.Command) error { return handleImagesModels(ctx, command, opts) }, }) return } @@ -65,30 +67,29 @@ type imageModelsReport struct { Models []imagemodels.Result `json:"models"` } -// Keep discovery's local guidance identifiable without retaining API errors or +// ModelsError keeps discovery's local guidance identifiable without retaining API errors or // rejected arguments. Only failure categories and the local help invocation // contribute to its message. -type imageModelsError struct { +type ModelsError struct { results []imagemodels.Result invocation string extraArguments bool } -func (e *imageModelsError) Error() string { +func (e *ModelsError) Error() string { if e.extraArguments { return fmt.Sprintf("use %s images models to see image model names; no additional arguments are needed", e.invocation) } return imageModelsFailureMessage(e.results, e.invocation) } -func handleImagesModels(ctx context.Context, command *cli.Command) error { +func handleImagesModels(ctx context.Context, command *cli.Command, opts ModelOptions) error { invocation := imageModelsInvocation(command) if command.Args().Present() { - return &imageModelsError{invocation: invocation, extraArguments: true} + return &ModelsError{invocation: invocation, extraArguments: true} } root := command.Root() - human := root.String("transform") == "" && !root.Bool("raw-output") && - resolvedOutputFormat(ShowJSONOpts{Format: root.String("format")}) == "text" + human := opts.HumanOutput(command) report := imageModelsReport{Source: "live", Complete: true} var results []imagemodels.Result if command.Bool("offline") { @@ -100,12 +101,12 @@ func handleImagesModels(ctx context.Context, command *cli.Command) error { // Parse request options once before parallel lookups, retaining normal // headers, authentication, project, endpoint and mTLS handling. Discovery // has no request body or stdin parameters. - options, err := FlagOptions(command, apiquery.NestedQueryFormatBrackets, apiquery.ArrayQueryFormatBrackets, EmptyBody, true) + options, err := opts.RequestOptions(command, apiquery.NestedQueryFormatBrackets, apiquery.ArrayQueryFormatBrackets, requestconfig.EmptyBody, true) if err != nil { return err } - client := openai.NewClient(GetDefaultRequestOptions(command)...) - if human && isTerminal(os.Stderr) && !root.Bool("debug") && outputDiagnosticsAllowed(ctx) { + client := openai.NewClient(opts.DefaultRequestOptions(command)...) + if human && cliterminal.IsWriterTerminal(os.Stderr) && !root.Bool("debug") && opts.DiagnosticsAllowed(ctx) { if _, err := fmt.Fprintln(os.Stderr, "Checking image models..."); err != nil { return err } @@ -124,7 +125,7 @@ func handleImagesModels(ctx context.Context, command *cli.Command) error { } if human { var hints io.Writer - if outputDiagnosticsAllowed(ctx) { + if opts.DiagnosticsAllowed(ctx) { hints = os.Stderr } if err := writeImageModels(root.Writer, report, command.Bool("all"), invocation, hints); err != nil { @@ -132,7 +133,7 @@ func handleImagesModels(ctx context.Context, command *cli.Command) error { } } else { // Presentation warnings use ShowJSON's stderr default, not the command error buffer. - if err := writeImageModelsData(ctx, command, report, nil); err != nil { + if err := writeImageModelsData(ctx, command, report, nil, opts); err != nil { return err } } @@ -140,27 +141,19 @@ func handleImagesModels(ctx context.Context, command *cli.Command) error { return ctx.Err() } if report.Source == "live" && !report.Complete { - return &imageModelsError{results: results, invocation: invocation} + return &ModelsError{results: results, invocation: invocation} } return nil } -func writeImageModelsData(ctx context.Context, command *cli.Command, report imageModelsReport, stderr io.Writer) error { - root := command.Root() +func writeImageModelsData(ctx context.Context, command *cli.Command, report imageModelsReport, stderr io.Writer, opts ModelOptions) error { payload, err := json.Marshal(report) if err != nil { return err } obj := gjson.ParseBytes(payload) - opts := ShowJSONOpts{ - Context: context.WithoutCancel(ctx), - Format: root.String("format"), ExplicitFormat: root.IsSet("format"), - RawOutput: root.Bool("raw-output"), Title: "Image models", Transform: root.String("transform"), - } - // The report includes completed checks even after discovery is canceled. - // Presentation therefore uses its own context instead of the canceled lookup. - opts.Stdout, opts.Stderr = root.Writer, stderr - return ShowJSON(obj, opts) + // Completed checks remain presentable after discovery cancellation. + return opts.ShowData(context.WithoutCancel(ctx), command, obj, stderr) } func writeImageModels(out io.Writer, report imageModelsReport, all bool, invocation string, hints io.Writer) error { diff --git a/internal/commands/images/image_models_integration_test.go b/internal/commands/images/image_models_integration_test.go new file mode 100644 index 000000000..8864bdd42 --- /dev/null +++ b/internal/commands/images/image_models_integration_test.go @@ -0,0 +1,168 @@ +package images_test + +import ( + "context" + "crypto/tls" + "crypto/x509" + "encoding/json" + "errors" + "fmt" + "io" + "log" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + + "github.com/openai/openai-cli/internal/commands/images" + "github.com/openai/openai-cli/internal/imagemodels" + mtlstest "github.com/openai/openai-cli/internal/testutil/mtls" + "github.com/openai/openai-cli/pkg/custom" + "github.com/urfave/cli/v3" +) + +func TestImageModelsErrorClassification(t *testing.T) { + const private = "synthetic-private-detail\x1b]52;c;synthetic-private-control\a" + for _, extraArguments := range []bool{false, true} { + diagnostic := images.ModelsErrorForTest("openai", extraArguments, []imagemodels.Result{ + {Entry: imagemodels.Entry{ID: private}, Status: imagemodels.StatusUnknown, Failure: imagemodels.Failure(private), ShutdownDate: private}, + {Status: imagemodels.StatusUnknown, Failure: imagemodels.FailureAuthentication}, + }) + wrapped := fmt.Errorf("%s: %w", private, diagnostic) + want := diagnostic.Error() + if got := imageErrorMessage(nil, wrapped); got != want || strings.Contains(got, "synthetic-private-") { + t.Errorf("typed diagnostic = %q, want controlled guidance %q", got, want) + } + for _, arbitrary := range []error{errors.New(want), errors.New(want + private), errors.New(private)} { + if got := imageErrorMessage(nil, arbitrary); got != "The command could not be completed. Check your arguments with --help." { + t.Errorf("untyped diagnostic became trusted: %q", got) + } + } + for _, format := range []string{"text", "json"} { + root := readableErrorTestCommand(t, "--format-error", format) + var out strings.Builder + if err := custom.ShowCommandError(root, wrapped, &out); err != nil { + t.Fatal(err) + } + message := strings.TrimSpace(out.String()) + if format == "json" { + var payload struct{ Message string } + if err := json.Unmarshal([]byte(message), &payload); err != nil { + t.Fatal(err) + } + message = payload.Message + } + if message != want { + t.Errorf("%s diagnostic = %q, want %q", format, message, want) + } + } + if got := imageErrorMessage(nil, errors.Join(wrapped, context.Canceled)); got != "Request canceled." { + t.Errorf("cancellation lost precedence: %q", got) + } + } +} + +func TestImageModelsUsesConfiguredMTLSClient(t *testing.T) { + t.Setenv("OPENAI_API_KEY", "synthetic-models-key") + pki := mtlstest.NewPKI(t) + clientCAs := x509.NewCertPool() + if !clientCAs.AppendCertsFromPEM(pki.RootPEM) { + t.Fatal("could not configure synthetic client CA") + } + var requests atomic.Int32 + server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + if r.TLS == nil || len(r.TLS.VerifiedChains) == 0 || len(r.TLS.VerifiedChains[0]) != 3 { + t.Error("model check lost the configured client certificate chain") + } + if r.Header.Get("Authorization") != "Bearer synthetic-models-key" || r.Method != http.MethodGet || !strings.HasPrefix(r.URL.Path, "/v1/models/") { + t.Error("model check changed the configured credentials, method or base path") + } + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"id":%q,"object":"model"}`, strings.TrimPrefix(r.URL.Path, "/v1/models/")) + })) + server.Config.ErrorLog = log.New(io.Discard, "", 0) + server.TLS = &tls.Config{ClientAuth: tls.RequireAndVerifyClientCert, ClientCAs: clientCAs, MinVersion: tls.VersionTLS12} + server.StartTLS() + t.Cleanup(server.Close) + configureTestServerTrust(t, server) + certFile, keyFile := mtlstest.WriteClientFiles(t, pki.ClientChainPEM, pki.ClientKeyPEM) + var out, stderr strings.Builder + root := &cli.Command{ + Name: "openai", Writer: &out, ErrWriter: &stderr, + Flags: []cli.Flag{&cli.StringFlag{Name: "base-url"}, &cli.StringFlag{Name: "format", Value: "json"}}, + Commands: []*cli.Command{{Name: "images"}}, + } + custom.ConfigureCommand(root) + if err := root.Run(t.Context(), []string{"openai", "--base-url", server.URL + "/v1/", + "--mtls-client-cert-file", certFile, "--mtls-client-key-file", keyFile, "images", "models"}); err != nil { + t.Fatal(err) + } + var report images.TestImageModelsReport + if err := json.Unmarshal([]byte(out.String()), &report); err != nil { + t.Fatal(err) + } + if requests.Load() != 9 || !report.Complete || len(report.Models) != 9 || stderr.Len() != 0 { + t.Fatalf("mTLS discovery did not complete all model checks: requests=%d; report=%+v; stderr=%q", requests.Load(), report, stderr.String()) + } +} + +func TestImageModelsDataFormatsAndExtraction(t *testing.T) { + report := images.TestImageModelsReport{Source: "offline", Models: []imagemodels.Result{images.TestImageModelTestRow("gpt-image-2.5-sunburst", imagemodels.StatusNotChecked, "")}} + for _, format := range []string{"auto", "text", "JSON", "jsonl", "raw", "yaml", "explore"} { + t.Run(format, func(t *testing.T) { + var out, stderr strings.Builder + command := imageModelsOutputCommand(&out, &stderr, report) + if err := command.Run(context.Background(), []string{"openai", "--format", format}); err != nil { + t.Fatal(err) + } + if !strings.Contains(out.String(), "gpt-image-2.5-sunburst") || !strings.Contains(out.String(), "not_checked") || strings.Contains(out.String(), "default") { + t.Fatalf("incorrect report: %q", out.String()) + } + if format == "auto" || format == "text" { + if json.Valid([]byte(out.String())) || !strings.Contains(out.String(), "Source: offline") { + t.Fatalf("expected text: %q", out.String()) + } + } else if format != "yaml" && !json.Valid([]byte(out.String())) { + t.Fatalf("invalid JSON: %q", out.String()) + } + if format == "explore" && !strings.Contains(stderr.String(), "falling back to 'json'") { + t.Fatalf("missing existing explore warning: %q", stderr.String()) + } + }) + } + for _, format := range []string{"auto", "text", "json", "jsonl", "raw", "yaml"} { + for _, raw := range []bool{false, true} { + var out, stderr strings.Builder + command := imageModelsOutputCommand(&out, &stderr, report) + args := []string{"openai", "--format", format, "--transform", "models.0.id"} + if raw { + args = append(args, "--raw-output") + } + if err := command.Run(context.Background(), args); err != nil { + t.Fatal(err) + } + want := "gpt-image-2.5-sunburst\n" + if !raw && (format == "auto" || format == "json" || format == "jsonl" || format == "raw") { + want = "\"gpt-image-2.5-sunburst\"\n" + } + if out.String() != want { + t.Errorf("format=%s raw=%v got %q want %q", format, raw, out.String(), want) + } + } + } + var stderr strings.Builder + want := errors.New("synthetic data output failure") + if err := imageModelsOutputCommand(errorSink{want}, &stderr, report).Run(context.Background(), []string{"openai", "--format", "json"}); !errors.Is(err, want) { + t.Errorf("lost output failure: %v", err) + } +} + +func imageModelsOutputCommand(out, stderr io.Writer, report images.TestImageModelsReport) *cli.Command { + return &cli.Command{Name: "openai", Writer: out, Flags: []cli.Flag{ + &cli.StringFlag{Name: "format", Value: "auto"}, &cli.StringFlag{Name: "transform"}, &cli.BoolFlag{Name: "raw-output"}, + }, Action: func(ctx context.Context, command *cli.Command) error { + return images.TestWriteImageModelsData(ctx, command, report, stderr, custom.ImageModelOptions()) + }} +} diff --git a/internal/commands/images/image_models_test.go b/internal/commands/images/image_models_test.go new file mode 100644 index 000000000..38e4e92e4 --- /dev/null +++ b/internal/commands/images/image_models_test.go @@ -0,0 +1,118 @@ +package images + +import ( + "errors" + "io" + "strings" + "testing" + + "github.com/openai/openai-cli/internal/imagemodels" +) + +func imageModelTestRow(id string, status imagemodels.Status, failure imagemodels.Failure) imagemodels.Result { + return imagemodels.Result{Entry: imagemodels.Entry{ID: id}, Status: status, Failure: failure} +} + +func TestImageModelsPresentationPartialAndHidden(t *testing.T) { + report := imageModelsReport{Source: "live", Models: []imagemodels.Result{ + imageModelTestRow("gpt-image-2.5-sunburst", imagemodels.StatusVisible, ""), + imageModelTestRow("gpt-image-2.5-flare", imagemodels.StatusUnknown, imagemodels.FailureTimeout), + imageModelTestRow("gpt-image-1", imagemodels.StatusNotVisible, ""), + imageModelTestRow("dall-e-2", imagemodels.StatusRetired, ""), + }} + var out strings.Builder + if err := writeImageModels(&out, report, false, "openai", &out); err != nil { + t.Fatal(err) + } + for _, want := range []string{"gpt-image-2.5-sunburst", "gpt-image-2.5-flare", "Could not check (timeout)", "2 retired or not visible", "openai images models --all", `openai images generate --prompt "A tiny orange robot" --model gpt-image-2.5-sunburst`, "generation permissions can differ"} { + if !strings.Contains(out.String(), want) { + t.Errorf("missing %q: %s", want, out.String()) + } + } + for _, unwanted := range []string{"gpt-image-1", "dall-e-2", "default", "No known", "{", "\x1b"} { + if strings.Contains(out.String(), unwanted) { + t.Errorf("unexpected %q: %s", unwanted, out.String()) + } + } + out.Reset() + if err := writeImageModels(&out, report, true, "openai", &out); err != nil { + t.Fatal(err) + } + if !strings.Contains(out.String(), "gpt-image-1") || !strings.Contains(out.String(), "dall-e-2") { + t.Fatalf("--all lost results: %s", out.String()) + } +} + +func TestImageModelsSuggestsOnlyVisibleOrUncheckedNames(t *testing.T) { + for _, status := range []imagemodels.Status{imagemodels.StatusVisible, imagemodels.StatusNotChecked, imagemodels.StatusRetired, imagemodels.StatusNotVisible, imagemodels.StatusUnknown} { + t.Run(string(status), func(t *testing.T) { + report := imageModelsReport{Source: "live", Models: []imagemodels.Result{imageModelTestRow("gpt-image-1", status, imagemodels.FailureTimeout)}} + if status == imagemodels.StatusNotChecked { + report.Source = "offline" + } + for _, all := range []bool{false, true} { + var out strings.Builder + if err := writeImageModels(&out, report, all, "openai", &out); err != nil { + t.Fatal(err) + } + eligible := status == imagemodels.StatusVisible || status == imagemodels.StatusNotChecked + if strings.Contains(out.String(), "Choose a model") != eligible { + t.Errorf("misleading suggestion: %s", out.String()) + } + if strings.Contains(out.String(), "default") { + t.Errorf("claimed an unshipped default: %s", out.String()) + } + if status == imagemodels.StatusUnknown && (strings.Contains(out.String(), "No known") || !strings.Contains(out.String(), "Could not check (timeout)")) { + t.Errorf("failed check claimed unavailable: %s", out.String()) + } + if status == imagemodels.StatusNotChecked && (!strings.Contains(out.String(), "access not checked (offline)") || strings.Contains(out.String(), "Visible")) { + t.Errorf("offline claimed visibility: %s", out.String()) + } + } + }) + } +} + +func TestImageModelsStatusAndFailureGuidance(t *testing.T) { + for _, tc := range []struct { + row imagemodels.Result + want string + }{ + {imagemodels.Result{Status: imagemodels.StatusVisible, ShutdownDate: "2099-01-01"}, "Visible (retires 2099-01-01)"}, + {imagemodels.Result{Status: imagemodels.StatusRetired, ShutdownDate: "2000-01-01"}, "Retired 2000-01-01"}, + {imagemodels.Result{Status: imagemodels.StatusUnknown, Failure: imagemodels.FailureAuthentication}, "Could not check (authentication)"}, + {imagemodels.Result{Status: imagemodels.StatusUnknown, Failure: imagemodels.FailureForbidden}, "Could not check (access denied)"}, + {imagemodels.Result{Status: imagemodels.StatusUnknown, Failure: imagemodels.FailureRateLimit}, "Could not check (rate limit)"}, + } { + if got := imageModelStatusText(tc.row); got != tc.want { + t.Errorf("status=%q; want %q", got, tc.want) + } + } + for failure, want := range map[imagemodels.Failure]string{ + imagemodels.FailureAuthentication: "did not accept authentication", imagemodels.FailureForbidden: "denied access", + imagemodels.FailureRateLimit: "rate-limited", imagemodels.FailureTimeout: "timed out", imagemodels.FailureServer: "API could not complete", + imagemodels.FailureNetwork: "Could not reach", imagemodels.FailureInvalidResponse: "unexpected model response", imagemodels.FailureCanceled: "could not be completed", + } { + message := imageModelsFailureMessage([]imagemodels.Result{{Status: imagemodels.StatusUnknown, Failure: failure}}, "openai") + if !strings.Contains(message, want) || !strings.Contains(message, "openai images models --offline") { + t.Errorf("failure %s: %s", failure, message) + } + } +} + +type imageModelsFailWriter struct{ err error } + +func (w imageModelsFailWriter) Write([]byte) (int, error) { return 0, w.err } + +func TestImageModelsWriteErrors(t *testing.T) { + want := errors.New("synthetic write failure") + for _, failure := range []error{want, nil} { + expected := failure + if expected == nil { + expected = io.ErrShortWrite + } + if got := writeImageModels(imageModelsFailWriter{failure}, imageModelsReport{}, false, "openai", nil); !errors.Is(got, expected) { + t.Fatalf("error=%v; want %v", got, expected) + } + } +} diff --git a/pkg/custom/image_picker.go b/internal/commands/images/image_picker.go similarity index 99% rename from pkg/custom/image_picker.go rename to internal/commands/images/image_picker.go index 62258dca2..80ce3e064 100644 --- a/pkg/custom/image_picker.go +++ b/internal/commands/images/image_picker.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" diff --git a/pkg/custom/image_picker_command.go b/internal/commands/images/image_picker_command.go similarity index 98% rename from pkg/custom/image_picker_command.go rename to internal/commands/images/image_picker_command.go index 1953ad4f3..ab329f497 100644 --- a/pkg/custom/image_picker_command.go +++ b/internal/commands/images/image_picker_command.go @@ -1,4 +1,4 @@ -package custom +package images import "strings" diff --git a/pkg/custom/image_picker_completion.go b/internal/commands/images/image_picker_completion.go similarity index 70% rename from pkg/custom/image_picker_completion.go rename to internal/commands/images/image_picker_completion.go index b283e6a1f..ecc79aa8d 100644 --- a/pkg/custom/image_picker_completion.go +++ b/internal/commands/images/image_picker_completion.go @@ -1,4 +1,4 @@ -package custom +package images import ( "slices" @@ -6,8 +6,8 @@ import ( "github.com/urfave/cli/v3" ) -// Keep the optional current-session hook on the existing completion command. -func configureImagePickerCompletion(root *cli.Command) { +// ConfigureCompletion keeps the optional current-session hook on the existing completion command. +func ConfigureCompletion(root *cli.Command) { completion := root.Command("@completion") if completion == nil { return diff --git a/pkg/custom/image_picker_draft_test.go b/internal/commands/images/image_picker_draft_test.go similarity index 99% rename from pkg/custom/image_picker_draft_test.go rename to internal/commands/images/image_picker_draft_test.go index 4460201f8..282baa07f 100644 --- a/pkg/custom/image_picker_draft_test.go +++ b/internal/commands/images/image_picker_draft_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" diff --git a/internal/commands/images/image_picker_first_run.go b/internal/commands/images/image_picker_first_run.go new file mode 100644 index 000000000..dabe5cdc5 --- /dev/null +++ b/internal/commands/images/image_picker_first_run.go @@ -0,0 +1,143 @@ +package images + +import ( + "context" + "os" + "os/exec" + "strings" + + "github.com/openai/openai-cli/internal/autocomplete" + cliterminal "github.com/openai/openai-cli/internal/terminal" +) + +// SetupShellOnFirstRun performs optional setup after assembly admits a background worker. +func SetupShellOnFirstRun(ctx context.Context, args []string) { + // The immediate shell may differ from the account's login shell. + shell := cliterminal.ParentShell(ctx) + if !FirstRunEligible(args, os.Getenv, true, true, true, shell) { + return + } + executable, err := os.Executable() + if err != nil { + return + } + onPath, err := exec.LookPath("openai") + if err != nil || !imagePickerSameExecutable(executable, onPath) { + return + } + targets, err := imagePickerShellTarget(ctx, shell, true, "") + if err != nil { + return + } + _ = setupImagePickerFirstRun(ctx, targets) +} + +// FirstRunEligible checks shell and invocation eligibility without consuming arguments. +func FirstRunEligible(args []string, getenv func(string) string, inputTTY, outputTTY, errorTTY bool, shell string) bool { + switch shell { + case "bash", "zsh", "fish": + default: + return false + } + if len(args) == 0 || !inputTTY || !outputTTY || !errorTTY || strings.EqualFold(getenv("TERM"), "dumb") || + getenv("OPENAI_PICKER_SHELL") != "" { + return false + } + for _, name := range []string{"CI", "GITHUB_ACTIONS", "GITLAB_CI", "TF_BUILD", "BUILDKITE", "JENKINS_URL", "TEAMCITY_VERSION"} { + value := strings.ToLower(getenv(name)) + if value != "" && value != "false" && value != "0" { + return false + } + } + // Conservatively ignore internal operations anywhere in argv. Inspecting + // these fixed tokens never parses, consumes, or rewrites request arguments. + for _, argument := range args[1:] { + switch argument { + case "__complete", "@completion", "@manpages", "--generate-shell-completion", "tokenizer", "codex": + return false + } + } + return true +} + +func imagePickerSameExecutable(executable, onPath string) bool { + current, err := os.Stat(executable) + if err != nil || !current.Mode().IsRegular() { + return false + } + resolved, err := os.Stat(onPath) + return err == nil && resolved.Mode().IsRegular() && os.SameFile(current, resolved) +} + +func setupImagePickerFirstRun(ctx context.Context, targets []autocomplete.PickerInstallation) error { + if err := ctx.Err(); err != nil || len(targets) == 0 { + return err + } + for _, target := range targets { + if target.Shell == autocomplete.CompletionStylePowershell { + return nil + } + } + missing, err := imagePickerFirstRunMissing(ctx, targets) + if err != nil { + return err + } + if len(missing) == 0 { + legacy, err := legacyImagePickerTabChoicePath(string(targets[0].Shell)) + if err != nil { + return err + } + declined, err := readImagePickerTabDecline(legacy) + if err != nil || !declined { + return err + } + } + directory, err := imagePickerTabChoiceDirectory() + if err != nil { + return err + } + return autocomplete.WithPickerSetupLock(ctx, directory, func() error { + if err := migrateImagePickerTabDecline(ctx, string(targets[0].Shell)); err != nil { + return err + } + // Recheck preference and every startup file after taking the same + // decision lock as explicit installation/removal. An opt-out or a + // changed profile while waiting must not be overwritten. + missing, err := imagePickerFirstRunMissing(ctx, targets) + if err != nil { + return err + } + for _, target := range missing { + if _, err := autocomplete.InstallPicker(ctx, target); err != nil { + return err + } + } + return nil + }) +} + +func imagePickerFirstRunMissing(ctx context.Context, targets []autocomplete.PickerInstallation) ([]autocomplete.PickerInstallation, error) { + if err := ctx.Err(); err != nil || len(targets) == 0 { + return nil, err + } + for _, target := range targets { + if target.Shell == autocomplete.CompletionStylePowershell { + return nil, nil + } + } + declined, err := imagePickerTabDeclined(string(targets[0].Shell)) + if err != nil || declined { + return nil, err + } + var missing []autocomplete.PickerInstallation + for _, target := range targets { + installed, err := autocomplete.IsPickerInstalled(ctx, target) + if err != nil { + return nil, err + } + if !installed { + missing = append(missing, target) + } + } + return missing, nil +} diff --git a/pkg/custom/image_picker_first_run_test.go b/internal/commands/images/image_picker_first_run_test.go similarity index 80% rename from pkg/custom/image_picker_first_run_test.go rename to internal/commands/images/image_picker_first_run_test.go index 7c88c8491..c9aef5d2f 100644 --- a/pkg/custom/image_picker_first_run_test.go +++ b/internal/commands/images/image_picker_first_run_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -7,7 +7,6 @@ import ( "path/filepath" "runtime" "strings" - "sync" "testing" "time" @@ -15,59 +14,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestImagePickerFirstRunBlockedWorkDoesNotBlockCommand(t *testing.T) { - for _, canceled := range []bool{false, true} { - t.Run(map[bool]string{false: "deadline", true: "cancellation"}[canceled], func(t *testing.T) { - ctx, cancel := context.WithCancel(t.Context()) - defer cancel() - started, unblock, foregroundDone, workerDone := make(chan struct{}), make(chan struct{}), make(chan struct{}), make(chan struct{}) - var release sync.Once - t.Cleanup(func() { release.Do(func() { close(unblock) }) }) - workerContext := make(chan context.Context, 1) - go func() { - waitForImagePickerFirstRun(ctx, func(ctx context.Context) { - workerContext <- ctx - close(started) - <-unblock // Models an OS operation that ignores context cancellation. - close(workerDone) - }) - close(foregroundDone) - }() - select { - case <-started: - case <-time.After(3 * time.Second): - t.Fatal("setup worker did not start") - } - if canceled { - cancel() - } - select { - case <-foregroundDone: - case <-time.After(2 * time.Second): - t.Fatal("optional blocked I/O prevented the foreground command from continuing") - } - require.Error(t, (<-workerContext).Err()) - secondStarted := make(chan struct{}, 1) - waitForImagePickerFirstRun(t.Context(), func(context.Context) { secondStarted <- struct{}{} }) - require.Empty(t, secondStarted, "a blocked worker must not accumulate more work") - release.Do(func() { close(unblock) }) - <-workerDone - // The worker releases admission after its callback returns. - require.Eventually(t, func() bool { return len(imagePickerFirstRunWorker) == 0 }, time.Second, time.Millisecond) - waitForImagePickerFirstRun(t.Context(), func(context.Context) { secondStarted <- struct{}{} }) - require.Len(t, secondStarted, 1, "finished work must release the slot") - }) - } -} - -func TestImagePickerFirstRunAlreadyCanceledDoesNotStartWork(t *testing.T) { - ctx, cancel := context.WithCancel(t.Context()) - cancel() - started := make(chan struct{}, 1) - waitForImagePickerFirstRun(ctx, func(context.Context) { started <- struct{}{} }) - require.Empty(t, started) -} - func TestImagePickerFirstRunEligibility(t *testing.T) { for _, test := range []struct { name string @@ -98,11 +44,11 @@ func TestImagePickerFirstRunEligibility(t *testing.T) { {"no argv", nil, nil, [3]bool{true, true, true}, false}, } { t.Run(test.name, func(t *testing.T) { - require.Equal(t, test.want, imagePickerFirstRunEligible(test.args, func(name string) string { return test.env[name] }, test.tty[0], test.tty[1], test.tty[2], "zsh")) + require.Equal(t, test.want, FirstRunEligible(test.args, func(name string) string { return test.env[name] }, test.tty[0], test.tty[1], test.tty[2], "zsh")) }) } - require.False(t, imagePickerFirstRunEligible([]string{"openai", "--help"}, func(string) string { return "" }, true, true, true, "")) - require.False(t, imagePickerFirstRunEligible([]string{"openai", "--help"}, func(string) string { return "" }, true, true, true, "pwsh")) + require.False(t, FirstRunEligible([]string{"openai", "--help"}, func(string) string { return "" }, true, true, true, "")) + require.False(t, FirstRunEligible([]string{"openai", "--help"}, func(string) string { return "" }, true, true, true, "pwsh")) } func TestImagePickerFirstRunRequiresExactPathExecutable(t *testing.T) { @@ -243,7 +189,7 @@ func TestImagePickerFirstRunContendedLockHonorsDeadline(t *testing.T) { case <-time.After(3 * time.Second): t.Fatal("could not acquire fixture setup lock") } - ctx, cancel := context.WithTimeout(t.Context(), imagePickerFirstRunTimeout) + ctx, cancel := context.WithTimeout(t.Context(), 250*time.Millisecond) defer cancel() started := time.Now() err = setupImagePickerFirstRun(ctx, targets) diff --git a/pkg/custom/image_picker_folder.go b/internal/commands/images/image_picker_folder.go similarity index 99% rename from pkg/custom/image_picker_folder.go rename to internal/commands/images/image_picker_folder.go index 2562160b4..126af6bd7 100644 --- a/pkg/custom/image_picker_folder.go +++ b/internal/commands/images/image_picker_folder.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" diff --git a/pkg/custom/image_picker_folder_test.go b/internal/commands/images/image_picker_folder_test.go similarity index 99% rename from pkg/custom/image_picker_folder_test.go rename to internal/commands/images/image_picker_folder_test.go index 898459144..525238643 100644 --- a/pkg/custom/image_picker_folder_test.go +++ b/internal/commands/images/image_picker_folder_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" diff --git a/pkg/custom/image_picker_generate.go b/internal/commands/images/image_picker_generate.go similarity index 97% rename from pkg/custom/image_picker_generate.go rename to internal/commands/images/image_picker_generate.go index 43144fa28..345c38729 100644 --- a/pkg/custom/image_picker_generate.go +++ b/internal/commands/images/image_picker_generate.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -18,6 +18,7 @@ import ( clioutput "github.com/openai/openai-cli/internal/output" "github.com/openai/openai-cli/internal/readable" "github.com/openai/openai-cli/internal/requestflag" + cliterminal "github.com/openai/openai-cli/internal/terminal" "github.com/openai/openai-go/v3" "github.com/urfave/cli/v3" ) @@ -104,7 +105,7 @@ func imagePickerCredentialFailure(err error) bool { // and terminals that opt out of styling retain the same readable plain layout. func writeImagePickerRequestMessage(ctx context.Context, out io.Writer, root *cli.Command, failure error, draftSaved bool) error { message := readable.Text(imagePickerRequestMessage(root, failure, draftSaved)) - if imagePickerCredentialFailure(failure) && isTerminal(out) && os.Getenv("NO_COLOR") == "" && os.Getenv("TERM") != "dumb" { + if imagePickerCredentialFailure(failure) && cliterminal.IsWriterTerminal(out) && os.Getenv("NO_COLOR") == "" && os.Getenv("TERM") != "dumb" { heading, body, _ := strings.Cut(message, "\n") message = lipgloss.NewStyle().Bold(true).Render(heading) + "\n" + body } @@ -170,7 +171,7 @@ func imagePickerRecoverableRequest(err error) bool { return false } switch failure := err.(type) { - case cli.ExitCoder, *imageSavingError, *clioutput.WriteError: + case cli.ExitCoder, *SavingError, *clioutput.WriteError: return false case interface{ Unwrap() []error }: causes := failure.Unwrap() diff --git a/pkg/custom/image_picker_generate_test.go b/internal/commands/images/image_picker_generate_test.go similarity index 99% rename from pkg/custom/image_picker_generate_test.go rename to internal/commands/images/image_picker_generate_test.go index 90d23dc0e..81c46f3ac 100644 --- a/pkg/custom/image_picker_generate_test.go +++ b/internal/commands/images/image_picker_generate_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" diff --git a/pkg/custom/image_picker_inline.go b/internal/commands/images/image_picker_inline.go similarity index 99% rename from pkg/custom/image_picker_inline.go rename to internal/commands/images/image_picker_inline.go index 06f36167c..3a976c6f5 100644 --- a/pkg/custom/image_picker_inline.go +++ b/internal/commands/images/image_picker_inline.go @@ -1,4 +1,4 @@ -package custom +package images import ( "errors" diff --git a/pkg/custom/image_picker_layout_test.go b/internal/commands/images/image_picker_layout_test.go similarity index 99% rename from pkg/custom/image_picker_layout_test.go rename to internal/commands/images/image_picker_layout_test.go index 814a14b6d..20c50a4bd 100644 --- a/pkg/custom/image_picker_layout_test.go +++ b/internal/commands/images/image_picker_layout_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "fmt" diff --git a/pkg/custom/image_picker_navigation_view_test.go b/internal/commands/images/image_picker_navigation_view_test.go similarity index 99% rename from pkg/custom/image_picker_navigation_view_test.go rename to internal/commands/images/image_picker_navigation_view_test.go index a92f7d454..3fd56d85e 100644 --- a/pkg/custom/image_picker_navigation_view_test.go +++ b/internal/commands/images/image_picker_navigation_view_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "fmt" diff --git a/pkg/custom/image_picker_repeat_test.go b/internal/commands/images/image_picker_repeat_test.go similarity index 99% rename from pkg/custom/image_picker_repeat_test.go rename to internal/commands/images/image_picker_repeat_test.go index 31ca6047e..445dbc33d 100644 --- a/pkg/custom/image_picker_repeat_test.go +++ b/internal/commands/images/image_picker_repeat_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" diff --git a/pkg/custom/image_picker_session.go b/internal/commands/images/image_picker_session.go similarity index 99% rename from pkg/custom/image_picker_session.go rename to internal/commands/images/image_picker_session.go index 9fabea71a..5bfc3f8aa 100644 --- a/pkg/custom/image_picker_session.go +++ b/internal/commands/images/image_picker_session.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" diff --git a/pkg/custom/image_picker_session_test.go b/internal/commands/images/image_picker_session_test.go similarity index 99% rename from pkg/custom/image_picker_session_test.go rename to internal/commands/images/image_picker_session_test.go index 1261dd374..b10b29b4d 100644 --- a/pkg/custom/image_picker_session_test.go +++ b/internal/commands/images/image_picker_session_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" diff --git a/pkg/custom/image_picker_shell_consent_test.go b/internal/commands/images/image_picker_shell_consent_test.go similarity index 75% rename from pkg/custom/image_picker_shell_consent_test.go rename to internal/commands/images/image_picker_shell_consent_test.go index af684d202..c3f606dbd 100644 --- a/pkg/custom/image_picker_shell_consent_test.go +++ b/internal/commands/images/image_picker_shell_consent_test.go @@ -1,4 +1,4 @@ -package custom +package images_test import ( "context" @@ -9,38 +9,33 @@ import ( "time" "github.com/openai/openai-cli/internal/autocomplete" + "github.com/openai/openai-cli/internal/commands/images" "github.com/stretchr/testify/require" ) -func pickerConsentConfig(t *testing.T, home, name string) { - t.Helper() - t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, name)) - t.Setenv("APPDATA", filepath.Join(home, name)) -} - func TestImagePickerShellConsentSurvivesConfigurationChanges(t *testing.T) { for _, shell := range []string{"bash", "zsh", "fish"} { t.Run(shell, func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) pickerConsentConfig(t, home, "config-a") for _, action := range []string{"--install-picker", "--uninstall-picker"} { _, err := runPickerShellSetup(t, t.Context(), shell, action) require.NoError(t, err) } - choice, err := imagePickerTabChoicePath(shell) + choice, err := images.TestImagePickerTabChoicePath(shell) require.NoError(t, err) require.Equal(t, filepath.Join(home, ".openai", "shell"), filepath.Dir(choice)) pickerConsentConfig(t, home, "config-b") - changedChoice, err := imagePickerTabChoicePath(shell) + changedChoice, err := images.TestImagePickerTabChoicePath(shell) require.NoError(t, err) require.Equal(t, choice, changedChoice) - targets, err := imagePickerShellTarget(t.Context(), shell, false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), shell, false, "") require.NoError(t, err) - before := firstRunFileSnapshot(t, home) + before := images.TestFirstRunFileSnapshot(t, home) for range 2 { - require.NoError(t, setupImagePickerFirstRun(t.Context(), targets)) + require.NoError(t, images.TestSetupImagePickerFirstRun(t.Context(), targets)) } - after := firstRunFileSnapshot(t, home) + after := images.TestFirstRunFileSnapshot(t, home) require.Len(t, after, len(before), "changing configuration roots must not reinstall declined setup") for path, info := range before { require.True(t, os.SameFile(info, after[path])) @@ -49,7 +44,7 @@ func TestImagePickerShellConsentSurvivesConfigurationChanges(t *testing.T) { _, err = runPickerShellSetup(t, t.Context(), shell, "--install-picker") require.NoError(t, err) pickerConsentConfig(t, home, "config-a") - declined, err := imagePickerTabDeclined(shell) + declined, err := images.TestImagePickerTabDeclined(shell) require.NoError(t, err) require.False(t, declined, "explicit re-enable applies across configuration roots") }) @@ -59,20 +54,20 @@ func TestImagePickerShellConsentSurvivesConfigurationChanges(t *testing.T) { func TestImagePickerShellConsentMigratesCurrentLegacyChoice(t *testing.T) { for _, shell := range []string{"bash", "zsh", "fish"} { t.Run(shell, func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) pickerConsentConfig(t, home, "config-a") - legacy, err := legacyImagePickerTabChoicePath(shell) + legacy, err := images.TestLegacyImagePickerTabChoicePath(shell) require.NoError(t, err) require.NoError(t, os.MkdirAll(filepath.Dir(legacy), 0700)) require.NoError(t, os.WriteFile(legacy, nil, 0600)) - targets, err := imagePickerShellTarget(t.Context(), shell, false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), shell, false, "") require.NoError(t, err) - require.NoError(t, setupImagePickerFirstRun(t.Context(), targets)) - choice, err := imagePickerTabChoicePath(shell) + require.NoError(t, images.TestSetupImagePickerFirstRun(t.Context(), targets)) + choice, err := images.TestImagePickerTabChoicePath(shell) require.NoError(t, err) info, err := os.Lstat(choice) require.NoError(t, err) - require.True(t, privateImagePickerState(info)) + require.True(t, images.TestPrivateImagePickerState(info)) require.Zero(t, info.Size()) _, err = os.Lstat(legacy) require.ErrorIs(t, err, os.ErrNotExist, "migrated consent must not revive after a later re-enable") @@ -81,13 +76,13 @@ func TestImagePickerShellConsentMigratesCurrentLegacyChoice(t *testing.T) { require.ErrorIs(t, err, os.ErrNotExist, "migration must preserve the user's opt-out") } pickerConsentConfig(t, home, "config-b") - targets, err = imagePickerShellTarget(t.Context(), shell, false, "") + targets, err = images.TestImagePickerShellTarget(t.Context(), shell, false, "") require.NoError(t, err) - require.NoError(t, setupImagePickerFirstRun(t.Context(), targets)) + require.NoError(t, images.TestSetupImagePickerFirstRun(t.Context(), targets)) _, err = runPickerShellSetup(t, t.Context(), shell, "--install-picker") require.NoError(t, err) pickerConsentConfig(t, home, "config-a") - declined, err := imagePickerTabDeclined(shell) + declined, err := images.TestImagePickerTabDeclined(shell) require.NoError(t, err) require.False(t, declined) }) @@ -100,8 +95,8 @@ func TestImagePickerShellConsentRejectsInvalidLegacyChoice(t *testing.T) { if runtime.GOOS == "windows" && (kind == "public" || kind == "symlink") { t.Skip("Unix mode or symlink semantics") } - home := pickerShellSetupHome(t) - legacy, err := legacyImagePickerTabChoicePath("zsh") + home := images.TestPickerShellSetupHome(t) + legacy, err := images.TestLegacyImagePickerTabChoicePath("zsh") require.NoError(t, err) require.NoError(t, os.MkdirAll(filepath.Dir(legacy), 0700)) switch kind { @@ -123,9 +118,9 @@ func TestImagePickerShellConsentRejectsInvalidLegacyChoice(t *testing.T) { } before, err := os.Lstat(legacy) require.NoError(t, err) - targets, err := imagePickerShellTarget(t.Context(), "zsh", false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), "zsh", false, "") require.NoError(t, err) - require.Error(t, setupImagePickerFirstRun(t.Context(), targets)) + require.Error(t, images.TestSetupImagePickerFirstRun(t.Context(), targets)) _, err = runPickerShellSetup(t, t.Context(), "zsh", "--install-picker") require.Error(t, err) after, err := os.Lstat(legacy) @@ -134,7 +129,7 @@ func TestImagePickerShellConsentRejectsInvalidLegacyChoice(t *testing.T) { require.Equal(t, before.ModTime(), after.ModTime()) _, err = os.Stat(targets[0].Profile) require.ErrorIs(t, err, os.ErrNotExist) - choice, err := imagePickerTabChoicePath("zsh") + choice, err := images.TestImagePickerTabChoicePath("zsh") require.NoError(t, err) _, err = os.Lstat(choice) require.ErrorIs(t, err, os.ErrNotExist, "invalid legacy state must not be silently migrated") @@ -145,9 +140,9 @@ func TestImagePickerShellConsentRejectsInvalidLegacyChoice(t *testing.T) { func TestImagePickerShellConsentDecisionLockAcrossConfigurationChanges(t *testing.T) { for _, action := range []string{"first run", "remove"} { t.Run(action, func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) pickerConsentConfig(t, home, "config-a") - choice, err := imagePickerTabChoicePath("zsh") + choice, err := images.TestImagePickerTabChoicePath("zsh") require.NoError(t, err) acquired, release, finished := make(chan struct{}), make(chan struct{}), make(chan error, 1) go func() { @@ -164,12 +159,12 @@ func TestImagePickerShellConsentDecisionLockAcrossConfigurationChanges(t *testin } defer func() { close(release); require.NoError(t, <-finished) }() pickerConsentConfig(t, home, "config-b") - targets, err := imagePickerShellTarget(t.Context(), "zsh", false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), "zsh", false, "") require.NoError(t, err) ctx, cancel := context.WithTimeout(t.Context(), 150*time.Millisecond) defer cancel() if action == "first run" { - err = setupImagePickerFirstRun(ctx, targets) + err = images.TestSetupImagePickerFirstRun(ctx, targets) } else { _, err = runPickerShellSetup(t, ctx, "zsh", "--uninstall-picker") } @@ -181,3 +176,9 @@ func TestImagePickerShellConsentDecisionLockAcrossConfigurationChanges(t *testin }) } } + +func pickerConsentConfig(t *testing.T, home, name string) { + t.Helper() + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, name)) + t.Setenv("APPDATA", filepath.Join(home, name)) +} diff --git a/pkg/custom/image_picker_shell_profile.go b/internal/commands/images/image_picker_shell_profile.go similarity index 99% rename from pkg/custom/image_picker_shell_profile.go rename to internal/commands/images/image_picker_shell_profile.go index 36ad51783..265956b4f 100644 --- a/pkg/custom/image_picker_shell_profile.go +++ b/internal/commands/images/image_picker_shell_profile.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" diff --git a/pkg/custom/image_picker_shell_profile_darwin_test.go b/internal/commands/images/image_picker_shell_profile_darwin_test.go similarity index 88% rename from pkg/custom/image_picker_shell_profile_darwin_test.go rename to internal/commands/images/image_picker_shell_profile_darwin_test.go index 0036813e0..4888c040d 100644 --- a/pkg/custom/image_picker_shell_profile_darwin_test.go +++ b/internal/commands/images/image_picker_shell_profile_darwin_test.go @@ -1,6 +1,6 @@ //go:build darwin -package custom +package images_test import ( "context" @@ -11,17 +11,18 @@ import ( "time" "github.com/openai/openai-cli/internal/autocomplete" + "github.com/openai/openai-cli/internal/commands/images" "github.com/stretchr/testify/require" ) func TestImagePickerMacOSShellConfigChangesShareDecisionLock(t *testing.T) { for _, shell := range []string{"zsh", "fish"} { t.Run(shell, func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, "config-a")) - first, err := imagePickerShellTarget(t.Context(), shell, false, "") + first, err := images.TestImagePickerShellTarget(t.Context(), shell, false, "") require.NoError(t, err) - choice, err := imagePickerTabChoicePath(shell) + choice, err := images.TestImagePickerTabChoicePath(shell) require.NoError(t, err) acquired, release := make(chan struct{}), make(chan struct{}) finished := make(chan error, 1) @@ -43,9 +44,9 @@ func TestImagePickerMacOSShellConfigChangesShareDecisionLock(t *testing.T) { require.NoError(t, <-finished) }() t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, "config-b")) - second, err := imagePickerShellTarget(t.Context(), shell, false, "") + second, err := images.TestImagePickerShellTarget(t.Context(), shell, false, "") require.NoError(t, err) - secondChoice, err := imagePickerTabChoicePath(shell) + secondChoice, err := images.TestImagePickerTabChoicePath(shell) require.NoError(t, err) require.Equal(t, choice, secondChoice) ctx, cancel := context.WithTimeout(t.Context(), 150*time.Millisecond) @@ -53,7 +54,7 @@ func TestImagePickerMacOSShellConfigChangesShareDecisionLock(t *testing.T) { output, err := runPickerShellSetup(t, ctx, shell, "--uninstall-picker") require.ErrorIs(t, err, context.DeadlineExceeded, "shared preferences require the same decision lock across XDG roots") require.Empty(t, output) - declined, err := imagePickerTabDeclined(shell) + declined, err := images.TestImagePickerTabDeclined(shell) require.NoError(t, err) require.False(t, declined, "a blocked uninstall must not change the shared preference") require.Equal(t, filepath.Join(home, ".openai", "shell"), filepath.Dir(choice)) @@ -70,17 +71,15 @@ func TestImagePickerMacOSShellConfigChangesShareDecisionLock(t *testing.T) { } func TestImagePickerMacOSShellSetupRetainsLegacyScriptRoot(t *testing.T) { - home := pickerShellSetupHome(t) - legacy, err := imagePickerSelectShellTargets("zsh", "", imagePickerShellPaths{ - home: home, config: filepath.Join(home, "old-xdg"), zdotdir: home, - }) + home := images.TestPickerShellSetupHome(t) + legacy, err := images.TestImagePickerSelectShellTargets("zsh", "", images.ShellPathsForTest(home, filepath.Join(home, "old-xdg"), home)) require.NoError(t, err) personal := []byte("# personal startup\n") require.NoError(t, os.WriteFile(legacy[0].Profile, personal, 0600)) installed, err := autocomplete.InstallPicker(t.Context(), legacy[0]) require.NoError(t, err) t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, "new-xdg")) - current, err := imagePickerShellTarget(t.Context(), "zsh", false, "") + current, err := images.TestImagePickerShellTarget(t.Context(), "zsh", false, "") require.NoError(t, err) require.NotEqual(t, legacy[0].Directory, current[0].Directory) _, err = runPickerShellSetup(t, t.Context(), "zsh", "--install-picker") @@ -111,14 +110,14 @@ func TestImagePickerMacOSFishOptOutAcrossConfigurationRoots(t *testing.T) { {"lazy", "set -g fish_key_bindings fish_vi_key_bindings\nfunction fish_user_key_bindings\n bind -M insert \\t custom_tab\n bind -M default \\t custom_tab\nend\n"}, } { t.Run(bindings.name, func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) firstConfig, secondConfig := filepath.Join(home, "fish-a"), filepath.Join(home, "fish-b") t.Setenv("XDG_CONFIG_HOME", firstConfig) _, err := runPickerShellSetup(t, t.Context(), "fish", "--install-picker") require.NoError(t, err) - targets, err := imagePickerShellTarget(t.Context(), "fish", false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), "fish", false, "") require.NoError(t, err) - optOut, err := imagePickerTabChoicePath("fish") + optOut, err := images.TestImagePickerTabChoicePath("fish") require.NoError(t, err) require.Equal(t, optOut, targets[0].OptOutPath) profileBefore, err := os.ReadFile(targets[0].Profile) @@ -149,7 +148,7 @@ func TestImagePickerMacOSFishOptOutAcrossConfigurationRoots(t *testing.T) { _, err := runPickerShellSetup(t, t.Context(), "fish", action) require.NoError(t, err) } - declined, err := imagePickerTabDeclined("fish") + declined, err := images.TestImagePickerTabDeclined("fish") require.NoError(t, err) require.Equal(t, !phase.enabled, declined) result := filepath.Join(t.TempDir(), "result") diff --git a/pkg/custom/image_picker_shell_profile_windows_test.go b/internal/commands/images/image_picker_shell_profile_integration_windows_test.go similarity index 80% rename from pkg/custom/image_picker_shell_profile_windows_test.go rename to internal/commands/images/image_picker_shell_profile_integration_windows_test.go index 87d52b7c1..b781cc419 100644 --- a/pkg/custom/image_picker_shell_profile_windows_test.go +++ b/internal/commands/images/image_picker_shell_profile_integration_windows_test.go @@ -1,17 +1,16 @@ //go:build windows -package custom +package images_test import ( "context" "os" - "os/user" "path/filepath" - "strings" "testing" "time" "github.com/openai/openai-cli/internal/autocomplete" + "github.com/openai/openai-cli/internal/commands/images" "github.com/stretchr/testify/require" ) @@ -20,7 +19,7 @@ func TestImagePickerWindowsShellHomeDefaultRefusesAmbiguousPaths(t *testing.T) { for _, action := range []string{"--install-picker", "--uninstall-picker"} { for _, variant := range []string{"other native home", "relative home", "MSYS home", "invalid home"} { t.Run(shell+"/"+action+"/"+variant, func(t *testing.T) { - profileHome := pickerShellSetupHome(t) + profileHome := images.TestPickerShellSetupHome(t) shellHome := t.TempDir() for _, home := range []string{profileHome, shellHome} { require.NoError(t, os.WriteFile(filepath.Join(home, ".bashrc"), []byte("# personal startup\n"), 0600)) @@ -35,7 +34,7 @@ func TestImagePickerWindowsShellHomeDefaultRefusesAmbiguousPaths(t *testing.T) { home += "\nother" } t.Setenv("HOME", home) - targets, err := imagePickerShellTarget(t.Context(), shell, false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), shell, false, "") require.ErrorContains(t, err, "--profile") require.Empty(t, targets) output, err := runPickerShellSetup(t, t.Context(), shell, action) @@ -58,12 +57,12 @@ func TestImagePickerWindowsShellHomeDefaultRefusesAmbiguousPaths(t *testing.T) { func TestImagePickerWindowsShellHomeAutomaticSkipsMismatch(t *testing.T) { for _, shell := range []string{"bash", "zsh", "fish"} { t.Run(shell, func(t *testing.T) { - profileHome := pickerShellSetupHome(t) + profileHome := images.TestPickerShellSetupHome(t) shellHome := t.TempDir() t.Setenv("HOME", shellHome) t.Setenv("SHELL", filepath.Join(profileHome, shell+".exe")) - // This is also the target selection used by first-run setup. - targets, err := imagePickerShellTarget(t.Context(), shell, true, "") + + targets, err := images.TestImagePickerShellTarget(t.Context(), shell, true, "") require.NoError(t, err) require.Empty(t, targets) output, err := runPickerShellSetup(t, t.Context(), "--install-picker", "--automatic") @@ -79,7 +78,7 @@ func TestImagePickerWindowsShellHomeAutomaticSkipsMismatch(t *testing.T) { } func TestImagePickerWindowsShellHomeExplicitOverride(t *testing.T) { - profileHome := pickerShellSetupHome(t) + profileHome := images.TestPickerShellSetupHome(t) shellHome := t.TempDir() t.Setenv("HOME", shellHome) profile := filepath.Join(shellHome, ".bashrc") @@ -108,40 +107,8 @@ func TestImagePickerWindowsShellHomeExplicitOverride(t *testing.T) { } } -func TestImagePickerWindowsShellHomeMatchingNativePaths(t *testing.T) { - for _, format := range []string{"native", "forward slashes", "upper case", "lower case", "unset"} { - t.Run(format, func(t *testing.T) { - home := pickerShellSetupHome(t) - if format == "forward slashes" { - t.Setenv("HOME", filepath.ToSlash(home)) - } else if format == "upper case" { - t.Setenv("HOME", strings.ToUpper(home)) - } else if format == "lower case" { - t.Setenv("HOME", strings.ToLower(home)) - } else if format == "unset" { - t.Setenv("HOME", "") - } - targets, err := imagePickerShellTarget(t.Context(), "bash", false, "") - require.NoError(t, err) - require.Len(t, targets, 2) - require.Equal(t, filepath.Join(home, ".bashrc"), targets[0].Profile) - require.Equal(t, filepath.Join(home, ".bash_profile"), targets[1].Profile) - }) - } -} - -func TestImagePickerWindowsShellHomeAutomaticUsesDirectoryIdentity(t *testing.T) { - home := t.TempDir() - current := &user.User{Uid: "1000", HomeDir: home} - for _, path := range []string{strings.ToUpper(home), strings.ToLower(home), filepath.ToSlash(home)} { - require.NoError(t, imagePickerAutomaticHome(path, current, 1000, false)) - } - require.Error(t, imagePickerAutomaticHome(t.TempDir(), current, 1000, false)) - require.Error(t, imagePickerAutomaticHome(filepath.Join(home, "absent"), current, 1000, false)) -} - func TestImagePickerWindowsShellHomeFishUsesConfiguredXDG(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) config := filepath.Join(home, "fish settings") appdata := filepath.Join(home, "native settings") t.Setenv("XDG_CONFIG_HOME", filepath.ToSlash(config)) @@ -155,7 +122,7 @@ func TestImagePickerWindowsShellHomeFishUsesConfiguredXDG(t *testing.T) { } nativeBefore, err := os.Stat(nativeProfile) require.NoError(t, err) - targets, err := imagePickerShellTarget(t.Context(), "fish", false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), "fish", false, "") require.NoError(t, err) require.Len(t, targets, 1) require.Equal(t, profile, targets[0].Profile) @@ -188,11 +155,11 @@ func TestImagePickerWindowsShellHomeFishUsesConfiguredXDG(t *testing.T) { } func TestImagePickerWindowsShellHomeFishFallsBackToHomeConfig(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) t.Setenv("XDG_CONFIG_HOME", "") appdata := filepath.Join(home, "native settings") t.Setenv("APPDATA", appdata) - targets, err := imagePickerShellTarget(t.Context(), "fish", false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), "fish", false, "") require.NoError(t, err) require.Len(t, targets, 1) require.Equal(t, filepath.Join(home, ".config", "fish", "conf.d", "openai-picker.fish"), targets[0].Profile) @@ -212,7 +179,7 @@ func TestImagePickerWindowsShellHomeFishFallsBackToHomeConfig(t *testing.T) { } func TestImagePickerWindowsShellHomeFishExplicitProfileKeepsAppDataStorage(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) shellHome := t.TempDir() config := filepath.Join(home, "fish settings") appdata := filepath.Join(home, "native settings") @@ -224,7 +191,7 @@ func TestImagePickerWindowsShellHomeFishExplicitProfileKeepsAppDataStorage(t *te require.NoError(t, os.WriteFile(profile, original, 0600)) _, err := runPickerShellSetup(t, t.Context(), "fish", "--install-picker", "--profile", profile) require.NoError(t, err) - targets, err := imagePickerShellTarget(t.Context(), "fish", false, profile) + targets, err := images.TestImagePickerShellTarget(t.Context(), "fish", false, profile) require.NoError(t, err) require.Len(t, targets, 1) require.Equal(t, profile, targets[0].Profile) @@ -241,11 +208,11 @@ func TestImagePickerWindowsShellHomeFishExplicitProfileKeepsAppDataStorage(t *te _, err := os.Stat(filepath.Join(directory, "fish")) require.ErrorIs(t, err, os.ErrNotExist, "an explicit profile must not create a default fish loader") } - // An override does not use the shell's default configuration directory. + t.Setenv("XDG_CONFIG_HOME", "relative-unused-config") _, err = runPickerShellSetup(t, t.Context(), "fish", "--install-picker", "--profile", profile) require.NoError(t, err) - targets, err = imagePickerShellTarget(t.Context(), "fish", false, profile) + targets, err = images.TestImagePickerShellTarget(t.Context(), "fish", false, profile) require.NoError(t, err) require.Equal(t, filepath.Join(appdata, "openai", "shell"), targets[0].Directory) _, err = runPickerShellSetup(t, t.Context(), "fish", "--uninstall-picker", "--profile", profile) @@ -255,9 +222,9 @@ func TestImagePickerWindowsShellHomeFishExplicitProfileKeepsAppDataStorage(t *te func TestImagePickerWindowsShellHomeFishRejectsInvalidXDG(t *testing.T) { for _, config := range []string{"relative-config", "/c/Users/synthetic-config", "C:drive-relative", "C:\\synthetic\nconfig"} { t.Run(config, func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) t.Setenv("XDG_CONFIG_HOME", config) - targets, err := imagePickerShellTarget(t.Context(), "fish", false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), "fish", false, "") require.Error(t, err) require.Empty(t, targets) for _, action := range []string{"--install-picker", "--uninstall-picker"} { @@ -275,12 +242,12 @@ func TestImagePickerWindowsShellHomeFishRejectsInvalidXDG(t *testing.T) { func TestImagePickerWindowsShellHomeOtherShellsKeepAppData(t *testing.T) { for _, shell := range []string{"bash", "zsh"} { t.Run(shell, func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) config := filepath.Join(home, "other XDG settings") appdata := filepath.Join(home, "native settings") t.Setenv("XDG_CONFIG_HOME", config) t.Setenv("APPDATA", appdata) - targets, err := imagePickerShellTarget(t.Context(), shell, false, "") + targets, err := images.TestImagePickerShellTarget(t.Context(), shell, false, "") require.NoError(t, err) for _, target := range targets { require.Equal(t, filepath.Join(appdata, "openai", "shell"), target.Directory) @@ -297,7 +264,7 @@ func TestImagePickerWindowsShellHomeOtherShellsKeepAppData(t *testing.T) { _, err = os.Stat(config) require.ErrorIs(t, err, os.ErrNotExist) t.Setenv("XDG_CONFIG_HOME", "relative-unused-config") - targets, err = imagePickerShellTarget(t.Context(), shell, false, "") + targets, err = images.TestImagePickerShellTarget(t.Context(), shell, false, "") require.NoError(t, err) for _, target := range targets { require.Equal(t, filepath.Join(appdata, "openai", "shell"), target.Directory) @@ -307,9 +274,9 @@ func TestImagePickerWindowsShellHomeOtherShellsKeepAppData(t *testing.T) { } func TestImagePickerWindowsShellHomeFishConfigChangesShareSetupLock(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, "fish-a")) - choice, err := imagePickerTabChoicePath("fish") + choice, err := images.TestImagePickerTabChoicePath("fish") require.NoError(t, err) acquired, release := make(chan struct{}), make(chan struct{}) finished := make(chan error, 1) @@ -335,7 +302,7 @@ func TestImagePickerWindowsShellHomeFishConfigChangesShareSetupLock(t *testing.T defer cancel() _, err = runPickerShellSetup(t, ctx, "fish", "--uninstall-picker") require.ErrorIs(t, err, context.DeadlineExceeded, "another fish configuration must wait on the same decision lock") - declined, err := imagePickerTabDeclined("fish") + declined, err := images.TestImagePickerTabDeclined("fish") require.NoError(t, err) require.False(t, declined, "a blocked setup decision must not update the shared preference") for _, config := range []string{"fish-a", "fish-b"} { @@ -345,33 +312,33 @@ func TestImagePickerWindowsShellHomeFishConfigChangesShareSetupLock(t *testing.T } func TestImagePickerWindowsShellHomeFishSharesOptOutAcrossRoots(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, "fish-a")) _, err := runPickerShellSetup(t, t.Context(), "fish", "--install-picker") require.NoError(t, err) - first, err := imagePickerShellTarget(t.Context(), "fish", false, "") + first, err := images.TestImagePickerShellTarget(t.Context(), "fish", false, "") require.NoError(t, err) - path, err := imagePickerTabChoicePath("fish") + path, err := images.TestImagePickerTabChoicePath("fish") require.NoError(t, err) require.Equal(t, path, first[0].OptOutPath) profile, err := os.ReadFile(first[0].Profile) require.NoError(t, err) t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, "fish-b")) - second, err := imagePickerShellTarget(t.Context(), "fish", false, "") + second, err := images.TestImagePickerShellTarget(t.Context(), "fish", false, "") require.NoError(t, err) require.NotEqual(t, first[0].Profile, second[0].Profile) require.Equal(t, first[0].OptOutPath, second[0].OptOutPath) for _, action := range []string{"--uninstall-picker", "--install-picker"} { _, err = runPickerShellSetup(t, t.Context(), "fish", action) require.NoError(t, err) - declined, err := imagePickerTabDeclined("fish") + declined, err := images.TestImagePickerTabDeclined("fish") require.NoError(t, err) require.Equal(t, action == "--uninstall-picker", declined) unchanged, err := os.ReadFile(first[0].Profile) require.NoError(t, err) require.Equal(t, profile, unchanged, "the old root must use the shared preference without a profile rewrite") } - explicit, err := imagePickerShellTarget(t.Context(), "fish", false, filepath.Join(home, "explicit.fish")) + explicit, err := images.TestImagePickerShellTarget(t.Context(), "fish", false, filepath.Join(home, "explicit.fish")) require.NoError(t, err) require.Equal(t, path, explicit[0].OptOutPath) } diff --git a/pkg/custom/image_picker_shell_profile_test.go b/internal/commands/images/image_picker_shell_profile_test.go similarity index 99% rename from pkg/custom/image_picker_shell_profile_test.go rename to internal/commands/images/image_picker_shell_profile_test.go index 5b8780eca..bd6007625 100644 --- a/pkg/custom/image_picker_shell_profile_test.go +++ b/internal/commands/images/image_picker_shell_profile_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" diff --git a/internal/commands/images/image_picker_shell_profile_windows_test.go b/internal/commands/images/image_picker_shell_profile_windows_test.go new file mode 100644 index 000000000..a3a34d10e --- /dev/null +++ b/internal/commands/images/image_picker_shell_profile_windows_test.go @@ -0,0 +1,42 @@ +package images + +import ( + "os/user" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestImagePickerWindowsShellHomeMatchingNativePaths(t *testing.T) { + for _, format := range []string{"native", "forward slashes", "upper case", "lower case", "unset"} { + t.Run(format, func(t *testing.T) { + home := pickerShellSetupHome(t) + if format == "forward slashes" { + t.Setenv("HOME", filepath.ToSlash(home)) + } else if format == "upper case" { + t.Setenv("HOME", strings.ToUpper(home)) + } else if format == "lower case" { + t.Setenv("HOME", strings.ToLower(home)) + } else if format == "unset" { + t.Setenv("HOME", "") + } + targets, err := imagePickerShellTarget(t.Context(), "bash", false, "") + require.NoError(t, err) + require.Len(t, targets, 2) + require.Equal(t, filepath.Join(home, ".bashrc"), targets[0].Profile) + require.Equal(t, filepath.Join(home, ".bash_profile"), targets[1].Profile) + }) + } +} + +func TestImagePickerWindowsShellHomeAutomaticUsesDirectoryIdentity(t *testing.T) { + home := t.TempDir() + current := &user.User{Uid: "1000", HomeDir: home} + for _, path := range []string{strings.ToUpper(home), strings.ToLower(home), filepath.ToSlash(home)} { + require.NoError(t, imagePickerAutomaticHome(path, current, 1000, false)) + } + require.Error(t, imagePickerAutomaticHome(t.TempDir(), current, 1000, false)) + require.Error(t, imagePickerAutomaticHome(filepath.Join(home, "absent"), current, 1000, false)) +} diff --git a/internal/commands/images/image_picker_shell_profile_zsh_integration_test.go b/internal/commands/images/image_picker_shell_profile_zsh_integration_test.go new file mode 100644 index 000000000..e046894f6 --- /dev/null +++ b/internal/commands/images/image_picker_shell_profile_zsh_integration_test.go @@ -0,0 +1,57 @@ +package images_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/openai/openai-cli/internal/commands/images" + "github.com/stretchr/testify/require" +) + +func TestImagePickerZshStartupChild(t *testing.T) { + if os.Getenv("OPENAI_TEST_ZSH_CHILD") != "1" { + t.Skip("native zsh subprocess helper") + } + mode := os.Getenv("OPENAI_TEST_ZSH_MODE") + if mode == "automatic" || mode == "default" && os.Getenv("OPENAI_TEST_ZSH_DEFAULT") != "allowed" { + t.Cleanup(func() { + state, err := images.TestImagePickerStatePath() + require.NoError(t, err) + choice, err := images.TestImagePickerTabChoicePath("zsh") + require.NoError(t, err) + for _, path := range []string{state, choice} { + _, err = os.Stat(filepath.Dir(path)) + require.ErrorIs(t, err, os.ErrNotExist, "ambiguous setup must not create platform state, scripts, or locks") + } + }) + } + if mode == "automatic" { + targets, err := images.TestImagePickerShellTarget(t.Context(), "zsh", true, "") + require.NoError(t, err) + require.Empty(t, targets, "ambiguous zsh state must skip before automatic setup") + require.NoError(t, images.TestSetupImagePickerFirstRun(t.Context(), targets)) + output, err := runPickerShellSetup(t, t.Context(), "--install-picker", "--automatic") + require.NoError(t, err) + require.Empty(t, output, "skipped automatic setup must not claim saved changes") + return + } + args := []string{"zsh", "--install-picker"} + if mode == "override" { + args = append(args, "--profile", os.Getenv("OPENAI_TEST_ZSH_PROFILE")) + } + output, err := runPickerShellSetup(t, t.Context(), args...) + if mode == "default" && os.Getenv("OPENAI_TEST_ZSH_DEFAULT") != "allowed" { + require.Error(t, err) + require.Contains(t, err.Error(), "--profile") + require.Empty(t, output) + return + } + require.NoError(t, err) + profile, err := os.ReadFile(os.Getenv("OPENAI_TEST_ZSH_PROFILE")) + require.NoError(t, err) + require.Contains(t, string(profile), "# >>> openai image picker") + args[1] = "--uninstall-picker" + _, err = runPickerShellSetup(t, t.Context(), args...) + require.NoError(t, err) +} diff --git a/pkg/custom/image_picker_shell_profile_zsh_test.go b/internal/commands/images/image_picker_shell_profile_zsh_test.go similarity index 62% rename from pkg/custom/image_picker_shell_profile_zsh_test.go rename to internal/commands/images/image_picker_shell_profile_zsh_test.go index 1e1c7b2f1..5c58ae7c6 100644 --- a/pkg/custom/image_picker_shell_profile_zsh_test.go +++ b/internal/commands/images/image_picker_shell_profile_zsh_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "os" @@ -9,53 +9,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestImagePickerZshStartupChild(t *testing.T) { - if os.Getenv("OPENAI_TEST_ZSH_CHILD") != "1" { - t.Skip("native zsh subprocess helper") - } - mode := os.Getenv("OPENAI_TEST_ZSH_MODE") - if mode == "automatic" || mode == "default" && os.Getenv("OPENAI_TEST_ZSH_DEFAULT") != "allowed" { - t.Cleanup(func() { - state, err := imagePickerStatePath() - require.NoError(t, err) - choice, err := imagePickerTabChoicePath("zsh") - require.NoError(t, err) - for _, path := range []string{state, choice} { - _, err = os.Stat(filepath.Dir(path)) - require.ErrorIs(t, err, os.ErrNotExist, "ambiguous setup must not create platform state, scripts, or locks") - } - }) - } - if mode == "automatic" { - targets, err := imagePickerShellTarget(t.Context(), "zsh", true, "") - require.NoError(t, err) - require.Empty(t, targets, "ambiguous zsh state must skip before automatic setup") - require.NoError(t, setupImagePickerFirstRun(t.Context(), targets)) - output, err := runPickerShellSetup(t, t.Context(), "--install-picker", "--automatic") - require.NoError(t, err) - require.Empty(t, output, "skipped automatic setup must not claim saved changes") - return - } - args := []string{"zsh", "--install-picker"} - if mode == "override" { - args = append(args, "--profile", os.Getenv("OPENAI_TEST_ZSH_PROFILE")) - } - output, err := runPickerShellSetup(t, t.Context(), args...) - if mode == "default" && os.Getenv("OPENAI_TEST_ZSH_DEFAULT") != "allowed" { - require.Error(t, err) - require.Contains(t, err.Error(), "--profile") - require.Empty(t, output) - return - } - require.NoError(t, err) - profile, err := os.ReadFile(os.Getenv("OPENAI_TEST_ZSH_PROFILE")) - require.NoError(t, err) - require.Contains(t, string(profile), "# >>> openai image picker") - args[1] = "--uninstall-picker" - _, err = runPickerShellSetup(t, t.Context(), args...) - require.NoError(t, err) -} - func TestImagePickerShellTargetNativeZshStartupEnvironment(t *testing.T) { zsh, err := exec.LookPath("zsh") if err != nil { @@ -95,6 +48,7 @@ func TestImagePickerShellTargetNativeZshStartupEnvironment(t *testing.T) { require.NoError(t, os.WriteFile(path, originals[path], 0600)) } require.NoError(t, os.WriteFile(filepath.Join(home, ".zshenv"), []byte(test.startup+"\n"), 0600)) + // The parent shell resolves this expression before launching the // child. '-' deliberately preserves an explicitly empty value. script := `export OPENAI_TEST_ZSH_PROFILE="${ZDOTDIR-$HOME}/.zshrc" diff --git a/pkg/custom/image_picker_shell_setup.go b/internal/commands/images/image_picker_shell_setup.go similarity index 97% rename from pkg/custom/image_picker_shell_setup.go rename to internal/commands/images/image_picker_shell_setup.go index 6f980dc95..75df8566a 100644 --- a/pkg/custom/image_picker_shell_setup.go +++ b/internal/commands/images/image_picker_shell_setup.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -67,9 +67,9 @@ func IsImagePickerShellSetupCommand(args []string) bool { return install || remove } -// Keep completion generation and persistent picker setup on the existing +// ConfigureShellSetup keeps completion generation and persistent picker setup on the existing // completion command, without changing the generated command definition. -func configureImagePickerShellSetup(root *cli.Command) { +func ConfigureShellSetup(root *cli.Command, receipt func(context.Context, string) error) { completion := root.Command("@completion") if completion == nil { return @@ -164,7 +164,7 @@ func configureImagePickerShellSetup(root *cli.Command) { } else if remove { message = "Tab shortcut setup removed. Open a new terminal to finish." } - return writeOutputReceipt(ctx, message) + return receipt(ctx, message) } } diff --git a/pkg/custom/image_picker_shell_setup_test.go b/internal/commands/images/image_picker_shell_setup_integration_test.go similarity index 80% rename from pkg/custom/image_picker_shell_setup_test.go rename to internal/commands/images/image_picker_shell_setup_integration_test.go index 33e8c5f88..77ab97587 100644 --- a/pkg/custom/image_picker_shell_setup_test.go +++ b/internal/commands/images/image_picker_shell_setup_integration_test.go @@ -1,4 +1,4 @@ -package custom +package images_test import ( "bytes" @@ -14,21 +14,12 @@ import ( "github.com/openai/openai-cli/internal/autocomplete" localcommands "github.com/openai/openai-cli/internal/commands" + "github.com/openai/openai-cli/internal/commands/images" + "github.com/openai/openai-cli/pkg/custom" "github.com/stretchr/testify/require" "github.com/urfave/cli/v3" ) -func pickerShellSetupHome(t *testing.T) string { - t.Helper() - home := t.TempDir() - for name, value := range map[string]string{ - "HOME": home, "USERPROFILE": home, "XDG_CONFIG_HOME": filepath.Join(home, "config"), "APPDATA": filepath.Join(home, "config"), "ZDOTDIR": home, - } { - t.Setenv(name, value) - } - return home -} - func runPickerShellSetup(t *testing.T, ctx context.Context, args ...string) (string, error) { t.Helper() output, _, err := runPickerShellSetupWithDiagnostics(t, ctx, args...) @@ -55,8 +46,8 @@ func runPickerShellSetupWithDiagnostics(t *testing.T, ctx context.Context, args ExitErrHandler: func(context.Context, *cli.Command, error) {}, Commands: []*cli.Command{{Name: "@completion", Action: autocomplete.OutputCompletionScript}}, } - configureImagePickerCompletion(root) - configureImagePickerShellSetup(root) + images.ConfigureCompletion(root) + images.ConfigureShellSetup(root, custom.ImagePreviewOptions().Receipt) err = root.Run(ctx, append([]string{"openai", "@completion"}, args...)) os.Stderr = previousStderr closeErr := diagnostics.Close() @@ -70,17 +61,17 @@ func runPickerShellSetupWithDiagnostics(t *testing.T, ctx context.Context, args func TestImagePickerShellSetupInstallRemoveExplicitProfile(t *testing.T) { for _, shell := range []string{"bash", "zsh", "fish"} { t.Run(shell, func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) profile := filepath.Join(home, "startup with spaces") original := []byte("# user-owned startup content\n") require.NoError(t, os.WriteFile(profile, original, 0600)) - require.NoError(t, declineImagePickerTab(t.Context(), shell)) + require.NoError(t, images.TestDeclineImagePickerTab(t.Context(), shell)) for range 2 { output, diagnostic, err := runPickerShellSetupWithDiagnostics(t, t.Context(), shell, "--install-picker", "--profile", profile) require.NoError(t, err) require.Empty(t, output) require.Equal(t, "Tab setup saved for future terminals. Existing custom bindings are preserved.\n", diagnostic) - targets, err := imagePickerShellTarget(t.Context(), shell, false, profile) + targets, err := images.TestImagePickerShellTarget(t.Context(), shell, false, profile) require.NoError(t, err) installed, err := autocomplete.IsPickerInstalled(t.Context(), targets[0]) require.NoError(t, err) @@ -89,7 +80,7 @@ func TestImagePickerShellSetupInstallRemoveExplicitProfile(t *testing.T) { require.NoError(t, err) require.True(t, bytes.HasPrefix(data, original)) require.Equal(t, 1, strings.Count(string(data), "# >>> openai image picker")) - declined, err := imagePickerTabDeclined(shell) + declined, err := images.TestImagePickerTabDeclined(shell) require.NoError(t, err) require.False(t, declined, "explicit installation must clear an old opt-out") } @@ -101,7 +92,7 @@ func TestImagePickerShellSetupInstallRemoveExplicitProfile(t *testing.T) { data, err := os.ReadFile(profile) require.NoError(t, err) require.Equal(t, original, data) - declined, err := imagePickerTabDeclined(shell) + declined, err := images.TestImagePickerTabDeclined(shell) require.NoError(t, err) require.True(t, declined, "removal must keep automatic setup off") } @@ -110,7 +101,7 @@ func TestImagePickerShellSetupInstallRemoveExplicitProfile(t *testing.T) { } func TestImagePickerShellSetupRejectsPowerShellWithoutWriting(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) profile := filepath.Join(home, "profile.ps1") for _, action := range []string{"--install-picker", "--uninstall-picker"} { for _, explicit := range []bool{false, true} { @@ -127,7 +118,7 @@ func TestImagePickerShellSetupRejectsPowerShellWithoutWriting(t *testing.T) { require.Empty(t, output) require.Empty(t, diagnostic) var presented bytes.Buffer - require.NoError(t, ShowCommandError(readableErrorTestCommand(t), err, &presented)) + require.NoError(t, custom.ShowCommandError(readableErrorTestCommand(t), err, &presented)) require.Contains(t, presented.String(), "PowerShell uses normal Tab completion") entries, err := os.ReadDir(home) require.NoError(t, err) @@ -145,7 +136,7 @@ func TestImagePickerShellSetupValidationPreservesRecoveryAndExit(t *testing.T) { {"conflicting actions", "Choose one setup action. Automatic setup does not accept a shell or profile override.", []string{"bash", "--install-picker", "--uninstall-picker", "--profile"}}, } { t.Run(tc.name, func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) profile := filepath.Join(home, "synthetic-private-profile") original := []byte("# preserved startup bytes\n") require.NoError(t, os.WriteFile(profile, original, 0600)) @@ -160,7 +151,7 @@ func TestImagePickerShellSetupValidationPreservesRecoveryAndExit(t *testing.T) { require.True(t, errors.Is(failure, local.Cause)) for _, format := range []string{"text", "json"} { var presented bytes.Buffer - require.NoError(t, ShowCommandError(readableErrorTestCommand(t, "--format-error", format), failure, &presented)) + require.NoError(t, custom.ShowCommandError(readableErrorTestCommand(t, "--format-error", format), failure, &presented)) require.Contains(t, presented.String(), tc.message) require.NotContains(t, presented.String(), "synthetic-private") } @@ -175,7 +166,7 @@ func TestImagePickerShellSetupValidationPreservesRecoveryAndExit(t *testing.T) { } func TestImagePickerShellSetupAutomaticPowerShellIsQuietAndDoesNotWrite(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) t.Setenv("SHELL", filepath.Join(home, "pwsh")) output, diagnostic, err := runPickerShellSetupWithDiagnostics(t, t.Context(), "--install-picker", "--automatic") require.NoError(t, err) @@ -187,7 +178,7 @@ func TestImagePickerShellSetupAutomaticPowerShellIsQuietAndDoesNotWrite(t *testi } func TestImagePickerShellSetupBashDefaultUpdatesBothStartupModes(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) profile := filepath.Join(home, ".profile") require.NoError(t, os.WriteFile(profile, []byte("# existing login startup\n"), 0600)) _, err := runPickerShellSetup(t, t.Context(), "bash", "--install-picker") @@ -204,7 +195,7 @@ func TestImagePickerShellSetupBashDefaultUpdatesBothStartupModes(t *testing.T) { func TestImagePickerShellSetupBashRemovalAfterLoginPrecedenceChanges(t *testing.T) { for _, reinstall := range []bool{false, true} { t.Run(fmt.Sprintf("reinstall=%t", reinstall), func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) originals := map[string][]byte{ ".bashrc": []byte("# interactive startup\n"), ".profile": []byte("# original login startup\n"), @@ -248,7 +239,7 @@ func TestImagePickerShellSetupBashRemovalAfterLoginPrecedenceChanges(t *testing. } func TestImagePickerShellSetupBashExplicitRemovalKeepsOtherProfiles(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) profile := filepath.Join(home, ".profile") original := []byte("# explicit removal target\n") require.NoError(t, os.WriteFile(profile, original, 0600)) @@ -265,7 +256,7 @@ func TestImagePickerShellSetupBashExplicitRemovalKeepsOtherProfiles(t *testing.T data, err = os.ReadFile(interactive) require.NoError(t, err) require.Equal(t, installed, data, "an explicit override must limit removal to that profile") - targets, err := imagePickerShellTarget(t.Context(), "bash", false, interactive) + targets, err := images.TestImagePickerShellTarget(t.Context(), "bash", false, interactive) require.NoError(t, err) kept, err := autocomplete.IsPickerInstalled(t.Context(), targets[0]) require.NoError(t, err) @@ -273,7 +264,7 @@ func TestImagePickerShellSetupBashExplicitRemovalKeepsOtherProfiles(t *testing.T } func TestImagePickerShellSetupPartialFailureIsRetryable(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) interactive, login := filepath.Join(home, ".bashrc"), filepath.Join(home, ".bash_profile") original := []byte("# personal startup\n") modified := []byte("# >>> openai image picker modified\n") @@ -289,6 +280,7 @@ func TestImagePickerShellSetupPartialFailureIsRetryable(t *testing.T) { data, err = os.ReadFile(login) require.NoError(t, err) require.Equal(t, modified, data) + // Simulate the user resolving the changed second profile before retrying. require.NoError(t, os.WriteFile(login, original, 0600)) _, err = runPickerShellSetup(t, t.Context(), "bash", "--install-picker") @@ -313,7 +305,7 @@ func TestImagePickerShellSetupBashRemovalContinuesAfterUnrelatedFailure(t *testi if runtime.GOOS == "windows" { t.Skip("Unix symlink and permission fixtures") } - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) profile := filepath.Join(home, ".profile") original := "# previous login settings\n" require.NoError(t, os.WriteFile(profile, []byte(original), 0600)) @@ -341,14 +333,14 @@ func TestImagePickerShellSetupBashRemovalContinuesAfterUnrelatedFailure(t *testi data, readErr := os.ReadFile(profile) require.NoError(t, readErr) require.Equal(t, original, string(data), "an independent unsafe file must not stop intact profile cleanup") - var failure *imageSavingError + var failure *images.SavingError require.ErrorAs(t, err, &failure) if unsafe == "symlink" { - require.ErrorContains(t, failure.cause, "requires a regular startup file") + require.ErrorContains(t, failure.Unwrap(), "requires a regular startup file") } else { - require.ErrorContains(t, failure.cause, "regular files writable only by their owner") + require.ErrorContains(t, failure.Unwrap(), "regular files writable only by their owner") } - require.ErrorContains(t, failure.cause, "ambiguous integration markers", "both independent failures must be retained") + require.ErrorContains(t, failure.Unwrap(), "ambiguous integration markers", "both independent failures must be retained") after, statErr := os.Lstat(preferred) require.NoError(t, statErr) require.True(t, os.SameFile(before, after)) @@ -365,7 +357,7 @@ func TestImagePickerShellSetupBashRemovalContinuesAfterUnrelatedFailure(t *testi scripts, globErr := filepath.Glob(filepath.Join(config, "openai", "shell", "picker-bash-*.bash")) require.NoError(t, globErr) require.Empty(t, scripts) - declined, preferenceErr := imagePickerTabDeclined("bash") + declined, preferenceErr := images.TestImagePickerTabDeclined("bash") require.NoError(t, preferenceErr) require.True(t, declined, "explicit removal must stay off after partial cleanup") } @@ -376,7 +368,7 @@ func TestImagePickerShellSetupBashRemovalContinuesAfterUnrelatedFailure(t *testi func TestImagePickerShellSetupRemovalRetainsPreferenceAndCleanupFailures(t *testing.T) { for _, modified := range []bool{false, true} { t.Run(fmt.Sprintf("modified=%t", modified), func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) profile := filepath.Join(home, ".zshrc") original := "# personal settings\n" require.NoError(t, os.WriteFile(profile, []byte(original), 0600)) @@ -386,18 +378,18 @@ func TestImagePickerShellSetupRemovalRetainsPreferenceAndCleanupFailures(t *test original = "# >>> openai image picker modified\n" require.NoError(t, os.WriteFile(profile, []byte(original), 0600)) } - preference, err := imagePickerTabChoicePath("zsh") + preference, err := images.TestImagePickerTabChoicePath("zsh") require.NoError(t, err) require.NoError(t, os.MkdirAll(preference, 0700)) output, diagnostic, err := runPickerShellSetupWithDiagnostics(t, t.Context(), "zsh", "--uninstall-picker") require.Error(t, err) require.Empty(t, output) require.Empty(t, diagnostic) - var failure *imageSavingError + var failure *images.SavingError require.ErrorAs(t, err, &failure) - require.ErrorContains(t, failure.cause, "invalid Tab shortcut preference") + require.ErrorContains(t, failure.Unwrap(), "invalid Tab shortcut preference") if modified { - require.ErrorContains(t, failure.cause, "ambiguous integration markers") + require.ErrorContains(t, failure.Unwrap(), "ambiguous integration markers") } data, readErr := os.ReadFile(profile) require.NoError(t, readErr) @@ -418,7 +410,7 @@ func TestImagePickerShellSetupRejectsInvalidActionCombinations(t *testing.T) { {"zsh", "--install-picker", "--profile", ""}, } { t.Run(strings.Join(args, " "), func(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) _, err := runPickerShellSetup(t, t.Context(), args...) var exit cli.ExitCoder require.ErrorAs(t, err, &exit) @@ -432,7 +424,7 @@ func TestImagePickerShellSetupRejectsInvalidActionCombinations(t *testing.T) { func TestImagePickerShellSetupUnknownShellAndAutomaticHomeDoNotWrite(t *testing.T) { for _, args := range [][]string{{"unknown", "--install-picker"}, {"--automatic", "--install-picker"}} { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) t.Setenv("SHELL", "/bin/zsh") output, diagnostic, err := runPickerShellSetupWithDiagnostics(t, t.Context(), args...) require.Error(t, err) @@ -445,7 +437,7 @@ func TestImagePickerShellSetupUnknownShellAndAutomaticHomeDoNotWrite(t *testing. } func TestImagePickerShellSetupCanceledBeforeActionDoesNotWrite(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) ctx, cancel := context.WithCancel(t.Context()) cancel() _, err := runPickerShellSetup(t, ctx, "zsh", "--install-picker") @@ -456,11 +448,11 @@ func TestImagePickerShellSetupCanceledBeforeActionDoesNotWrite(t *testing.T) { } func TestImagePickerShellSetupCancellationWhileAnotherSetupHoldsLock(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) profile := filepath.Join(home, "startup") original := []byte("# preserve while waiting for another setup\n") require.NoError(t, os.WriteFile(profile, original, 0600)) - choice, err := imagePickerTabChoicePath("zsh") + choice, err := images.TestImagePickerTabChoicePath("zsh") require.NoError(t, err) locked, release := make(chan struct{}), make(chan struct{}) done := make(chan error, 1) @@ -496,13 +488,13 @@ func TestImagePickerShellSetupCancellationWhileAnotherSetupHoldsLock(t *testing. data, err := os.ReadFile(profile) require.NoError(t, err) require.Equal(t, original, data, "a blocked setup must not edit profiles before acquiring the shared lock") - declined, err := imagePickerTabDeclined("zsh") + declined, err := images.TestImagePickerTabDeclined("zsh") require.NoError(t, err) require.False(t, declined) } func TestImagePickerShellSetupPreservesCompletionOutput(t *testing.T) { - home := pickerShellSetupHome(t) + home := images.TestPickerShellSetupHome(t) normal, normalDiagnostic, err := runPickerShellSetupWithDiagnostics(t, t.Context(), "zsh") require.NoError(t, err) require.Empty(t, normalDiagnostic) @@ -516,72 +508,3 @@ func TestImagePickerShellSetupPreservesCompletionOutput(t *testing.T) { require.NoError(t, err) require.Empty(t, entries, "emitting scripts must not install or save a choice") } - -func TestImagePickerTabChoicePrivateIdempotentAndCanceled(t *testing.T) { - pickerShellSetupHome(t) - path, err := imagePickerTabChoicePath("zsh") - require.NoError(t, err) - ctx, cancel := context.WithCancel(t.Context()) - cancel() - require.ErrorIs(t, declineImagePickerTab(ctx, "zsh"), context.Canceled) - _, err = os.Lstat(filepath.Dir(path)) - require.ErrorIs(t, err, os.ErrNotExist) - for range 2 { - require.NoError(t, declineImagePickerTab(t.Context(), "zsh")) - declined, err := imagePickerTabDeclined("zsh") - require.NoError(t, err) - require.True(t, declined) - } - info, err := os.Lstat(path) - require.NoError(t, err) - require.True(t, info.Mode().IsRegular()) - require.Zero(t, info.Size()) - if runtime.GOOS != "windows" { - require.Equal(t, os.FileMode(0600), info.Mode().Perm()) - } - for range 2 { - require.NoError(t, clearImagePickerTabDecline("zsh")) - } - declined, err := imagePickerTabDeclined("zsh") - require.NoError(t, err) - require.False(t, declined) -} - -func TestImagePickerTabChoiceRejectsModifiedOrSymlinkMarker(t *testing.T) { - for _, kind := range []string{"content", "directory", "public", "symlink"} { - t.Run(kind, func(t *testing.T) { - if runtime.GOOS == "windows" && (kind == "public" || kind == "symlink") { - t.Skip("Unix mode or symlink semantics") - } - home := pickerShellSetupHome(t) - path, err := imagePickerTabChoicePath("zsh") - require.NoError(t, err) - require.NoError(t, os.MkdirAll(filepath.Dir(path), 0700)) - target := filepath.Join(home, "preserved-file") - require.NoError(t, os.WriteFile(target, []byte("preserve these bytes"), 0600)) - switch kind { - case "content": - require.NoError(t, os.WriteFile(path, []byte("external content"), 0600)) - case "directory": - require.NoError(t, os.Mkdir(path, 0700)) - case "public": - require.NoError(t, os.WriteFile(path, nil, 0600)) - require.NoError(t, os.Chmod(path, 0644)) - case "symlink": - require.NoError(t, os.Symlink(target, path)) - } - before, err := os.Lstat(path) - require.NoError(t, err) - _, err = imagePickerTabDeclined("zsh") - require.Error(t, err) - require.Error(t, declineImagePickerTab(t.Context(), "zsh")) - require.Error(t, clearImagePickerTabDecline("zsh")) - after, err := os.Lstat(path) - require.NoError(t, err) - require.True(t, os.SameFile(before, after)) - data, err := os.ReadFile(target) - require.NoError(t, err) - require.Equal(t, "preserve these bytes", string(data)) - }) - } -} diff --git a/internal/commands/images/image_picker_shell_setup_test.go b/internal/commands/images/image_picker_shell_setup_test.go new file mode 100644 index 000000000..a3e51511f --- /dev/null +++ b/internal/commands/images/image_picker_shell_setup_test.go @@ -0,0 +1,91 @@ +package images + +import ( + "context" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/require" +) + +func pickerShellSetupHome(t *testing.T) string { + t.Helper() + home := t.TempDir() + for name, value := range map[string]string{ + "HOME": home, "USERPROFILE": home, "XDG_CONFIG_HOME": filepath.Join(home, "config"), "APPDATA": filepath.Join(home, "config"), "ZDOTDIR": home, + } { + t.Setenv(name, value) + } + return home +} + +func TestImagePickerTabChoicePrivateIdempotentAndCanceled(t *testing.T) { + pickerShellSetupHome(t) + path, err := imagePickerTabChoicePath("zsh") + require.NoError(t, err) + ctx, cancel := context.WithCancel(t.Context()) + cancel() + require.ErrorIs(t, declineImagePickerTab(ctx, "zsh"), context.Canceled) + _, err = os.Lstat(filepath.Dir(path)) + require.ErrorIs(t, err, os.ErrNotExist) + for range 2 { + require.NoError(t, declineImagePickerTab(t.Context(), "zsh")) + declined, err := imagePickerTabDeclined("zsh") + require.NoError(t, err) + require.True(t, declined) + } + info, err := os.Lstat(path) + require.NoError(t, err) + require.True(t, info.Mode().IsRegular()) + require.Zero(t, info.Size()) + if runtime.GOOS != "windows" { + require.Equal(t, os.FileMode(0600), info.Mode().Perm()) + } + for range 2 { + require.NoError(t, clearImagePickerTabDecline("zsh")) + } + declined, err := imagePickerTabDeclined("zsh") + require.NoError(t, err) + require.False(t, declined) +} + +func TestImagePickerTabChoiceRejectsModifiedOrSymlinkMarker(t *testing.T) { + for _, kind := range []string{"content", "directory", "public", "symlink"} { + t.Run(kind, func(t *testing.T) { + if runtime.GOOS == "windows" && (kind == "public" || kind == "symlink") { + t.Skip("Unix mode or symlink semantics") + } + home := pickerShellSetupHome(t) + path, err := imagePickerTabChoicePath("zsh") + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0700)) + target := filepath.Join(home, "preserved-file") + require.NoError(t, os.WriteFile(target, []byte("preserve these bytes"), 0600)) + switch kind { + case "content": + require.NoError(t, os.WriteFile(path, []byte("external content"), 0600)) + case "directory": + require.NoError(t, os.Mkdir(path, 0700)) + case "public": + require.NoError(t, os.WriteFile(path, nil, 0600)) + require.NoError(t, os.Chmod(path, 0644)) + case "symlink": + require.NoError(t, os.Symlink(target, path)) + } + before, err := os.Lstat(path) + require.NoError(t, err) + _, err = imagePickerTabDeclined("zsh") + require.Error(t, err) + require.Error(t, declineImagePickerTab(t.Context(), "zsh")) + require.Error(t, clearImagePickerTabDecline("zsh")) + after, err := os.Lstat(path) + require.NoError(t, err) + require.True(t, os.SameFile(before, after)) + data, err := os.ReadFile(target) + require.NoError(t, err) + require.Equal(t, "preserve these bytes", string(data)) + }) + } +} diff --git a/pkg/custom/image_picker_state.go b/internal/commands/images/image_picker_state.go similarity index 99% rename from pkg/custom/image_picker_state.go rename to internal/commands/images/image_picker_state.go index db9fcf5d1..9f577d462 100644 --- a/pkg/custom/image_picker_state.go +++ b/internal/commands/images/image_picker_state.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" diff --git a/pkg/custom/image_picker_state_access_other_test.go b/internal/commands/images/image_picker_state_access_other_test.go similarity index 93% rename from pkg/custom/image_picker_state_access_other_test.go rename to internal/commands/images/image_picker_state_access_other_test.go index eff3ea601..3e3b255e8 100644 --- a/pkg/custom/image_picker_state_access_other_test.go +++ b/internal/commands/images/image_picker_state_access_other_test.go @@ -1,6 +1,6 @@ //go:build !windows -package custom +package images import ( "os" diff --git a/pkg/custom/image_picker_state_access_windows_test.go b/internal/commands/images/image_picker_state_access_windows_test.go similarity index 99% rename from pkg/custom/image_picker_state_access_windows_test.go rename to internal/commands/images/image_picker_state_access_windows_test.go index cd9866d4f..3aa986f39 100644 --- a/pkg/custom/image_picker_state_access_windows_test.go +++ b/internal/commands/images/image_picker_state_access_windows_test.go @@ -1,6 +1,6 @@ //go:build windows -package custom +package images import ( "errors" diff --git a/pkg/custom/image_picker_state_test.go b/internal/commands/images/image_picker_state_test.go similarity index 99% rename from pkg/custom/image_picker_state_test.go rename to internal/commands/images/image_picker_state_test.go index c7ba20624..982ae1cf9 100644 --- a/pkg/custom/image_picker_state_test.go +++ b/internal/commands/images/image_picker_state_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" diff --git a/pkg/custom/image_picker_test.go b/internal/commands/images/image_picker_test.go similarity index 99% rename from pkg/custom/image_picker_test.go rename to internal/commands/images/image_picker_test.go index 8892abd5d..f270e7da7 100644 --- a/pkg/custom/image_picker_test.go +++ b/internal/commands/images/image_picker_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" diff --git a/pkg/custom/image_picker_view.go b/internal/commands/images/image_picker_view.go similarity index 99% rename from pkg/custom/image_picker_view.go rename to internal/commands/images/image_picker_view.go index 8f67d24f0..2cb8d223f 100644 --- a/pkg/custom/image_picker_view.go +++ b/internal/commands/images/image_picker_view.go @@ -1,4 +1,4 @@ -package custom +package images import ( "fmt" diff --git a/pkg/custom/image_picker_view_test.go b/internal/commands/images/image_picker_view_test.go similarity index 99% rename from pkg/custom/image_picker_view_test.go rename to internal/commands/images/image_picker_view_test.go index 2ffc56b6b..73b1d5a9b 100644 --- a/pkg/custom/image_picker_view_test.go +++ b/internal/commands/images/image_picker_view_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "fmt" diff --git a/pkg/custom/image_preferences.go b/internal/commands/images/image_preferences.go similarity index 96% rename from pkg/custom/image_preferences.go rename to internal/commands/images/image_preferences.go index 636987e9b..4cbd2ac81 100644 --- a/pkg/custom/image_preferences.go +++ b/internal/commands/images/image_preferences.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -49,7 +49,7 @@ func imageInlineMode(ctx context.Context, command *cli.Command, terminal bool) ( return imageprefs.Load(ctx, path) } -func imageInlinePreferenceCommands() []*cli.Command { +func imageInlinePreferenceCommands(receipt func(context.Context, string) error) []*cli.Command { var commands []*cli.Command for _, mode := range []string{"on", "off"} { commands = append(commands, &cli.Command{ @@ -73,7 +73,7 @@ func imageInlinePreferenceCommands() []*cli.Command { if mode == "on" { message += "\nOn also permits image-font activation in local Apple Terminal." } - return writeOutputReceipt(ctx, message) + return receipt(ctx, message) }, }) } diff --git a/pkg/custom/image_preferences_test.go b/internal/commands/images/image_preferences_integration_test.go similarity index 65% rename from pkg/custom/image_preferences_test.go rename to internal/commands/images/image_preferences_integration_test.go index 81e08eb37..e2a465073 100644 --- a/pkg/custom/image_preferences_test.go +++ b/internal/commands/images/image_preferences_integration_test.go @@ -1,4 +1,4 @@ -package custom +package images_test import ( "bytes" @@ -10,66 +10,18 @@ import ( "strings" "testing" - "github.com/openai/openai-cli/internal/imageprefs" + "github.com/openai/openai-cli/internal/commands/images" + "github.com/openai/openai-cli/pkg/custom" "github.com/stretchr/testify/require" "github.com/urfave/cli/v3" ) -func TestImageInlinePreferencePrecedence(t *testing.T) { - home := t.TempDir() - t.Setenv("HOME", home) - t.Setenv("APPDATA", home) - t.Setenv("XDG_CONFIG_HOME", home) - path, err := imageInlinePreferencePath() - require.NoError(t, err) - for _, preset := range []string{"absent", "on", "off", "corrupt"} { - if preset != "absent" { - require.NoError(t, os.MkdirAll(filepath.Dir(path), 0700)) - } - switch preset { - case "on", "off": - require.NoError(t, imageprefs.Save(t.Context(), path, preset == "on")) - case "corrupt": - require.NoError(t, os.WriteFile(path, []byte(`{"version":999,"inline":true}`), 0600)) - } - for _, flag := range []string{"", "auto", "on", "off"} { - for _, terminal := range []bool{false, true} { - name := preset + "/" + flag - t.Run(name, func(t *testing.T) { - app := &cli.Command{Name: "openai", Flags: []cli.Flag{&cli.StringFlag{Name: "inline", Value: "auto"}}, Action: func(ctx context.Context, command *cli.Command) error { - got, err := imageInlineMode(ctx, command, terminal) - if flag == "" && terminal && preset == "corrupt" { - require.Error(t, err) - return nil - } - require.NoError(t, err) - want := flag - if want == "" { - want = "auto" - if terminal && (preset == "on" || preset == "off") { - want = preset - } - } - require.Equal(t, want, got) - return nil - }} - args := []string{"openai"} - if flag != "" { - args = append(args, "--inline", flag) - } - require.NoError(t, app.Run(t.Context(), args)) - }) - } - } - } -} - func TestImageInlinePreferenceFailureKeepsPrivatePathsAndOriginal(t *testing.T) { home := filepath.Join(t.TempDir(), "synthetic-private-home") for _, name := range []string{"HOME", "APPDATA", "USERPROFILE", "XDG_CONFIG_HOME"} { t.Setenv(name, home) } - path, err := imageInlinePreferencePath() + path, err := images.TestImageInlinePreferencePath() require.NoError(t, err) blocked := filepath.Dir(path) require.NoError(t, os.MkdirAll(filepath.Dir(blocked), 0700)) @@ -78,7 +30,7 @@ func TestImageInlinePreferenceFailureKeepsPrivatePathsAndOriginal(t *testing.T) for _, mode := range []string{"on", "off"} { root := &cli.Command{Name: "openai", Writer: &bytes.Buffer{}, ErrWriter: &bytes.Buffer{}, Flags: []cli.Flag{&cli.StringFlag{Name: "format", Value: "auto"}}, - Commands: []*cli.Command{{Name: "images", Commands: []*cli.Command{{Name: "inline", Commands: imageInlinePreferenceCommands()}}}}, + Commands: []*cli.Command{{Name: "images", Commands: []*cli.Command{{Name: "inline", Commands: images.TestImageInlinePreferenceCommands(custom.ImagePreviewOptions().Receipt)}}}}, } failure := root.Run(t.Context(), []string{"openai", "images", "inline", mode}) var cause *os.PathError @@ -86,10 +38,10 @@ func TestImageInlinePreferenceFailureKeepsPrivatePathsAndOriginal(t *testing.T) require.ErrorIs(t, failure, cause) for _, format := range []string{"text", "json"} { var diagnostic bytes.Buffer - require.NoError(t, ShowCommandError(readableErrorTestCommand(t, "--format-error", format), failure, &diagnostic)) + require.NoError(t, custom.ShowCommandError(readableErrorTestCommand(t, "--format-error", format), failure, &diagnostic)) require.Contains(t, diagnostic.String(), "Existing settings were kept.") require.Contains(t, diagnostic.String(), "image-preferences.json") - location := imageInlinePreferenceLocation(runtime.GOOS) + location := images.TestImageInlinePreferenceLocation(runtime.GOOS) if format == "json" { location = strings.ReplaceAll(location, `\`, `\\`) } @@ -103,32 +55,15 @@ func TestImageInlinePreferenceFailureKeepsPrivatePathsAndOriginal(t *testing.T) } } -func TestImageInlinePreferenceLocationUsesSymbolicPaths(t *testing.T) { - for _, name := range []string{"HOME", "APPDATA", "USERPROFILE", "XDG_CONFIG_HOME"} { - t.Setenv(name, "synthetic-private-config") - } - for _, tc := range []struct{ goos, want string }{ - {"darwin", "$HOME/Library/Application Support/openai/image-preferences.json"}, - {"windows", `%AppData%\openai\image-preferences.json`}, - {"linux", "$XDG_CONFIG_HOME/openai/image-preferences.json, or $HOME/.config/openai/image-preferences.json when XDG_CONFIG_HOME is unset"}, - {"freebsd", "$XDG_CONFIG_HOME/openai/image-preferences.json, or $HOME/.config/openai/image-preferences.json when XDG_CONFIG_HOME is unset"}, - } { - t.Run(tc.goos, func(t *testing.T) { - require.Equal(t, tc.want, imageInlinePreferenceLocation(tc.goos)) - require.NotContains(t, imageInlinePreferenceLocation(tc.goos), "synthetic-private") - }) - } -} - func TestImageInlinePreferenceWarningTerminal(t *testing.T) { - if !isTerminal(os.Stdout) { + if !images.TestIsTerminal(os.Stdout) { t.Skip("requires actual terminal stdout") } home := t.TempDir() t.Setenv("HOME", home) t.Setenv("APPDATA", home) t.Setenv("XDG_CONFIG_HOME", home) - path, err := imageInlinePreferencePath() + path, err := images.TestImageInlinePreferencePath() require.NoError(t, err) require.NoError(t, os.MkdirAll(filepath.Dir(path), 0700)) require.NoError(t, os.WriteFile(path, []byte("invalid-private-settings"), 0600)) @@ -148,13 +83,12 @@ func TestImageInlinePreferenceWarningTerminal(t *testing.T) { defer cancel() called := false command := &cli.Command{Name: "generate", Reader: strings.NewReader(""), Writer: os.Stdout, - Action: imageSavingWorkflow(func(ctx context.Context, command *cli.Command) error { + Action: images.TestImageSavingWorkflow(func(ctx context.Context, command *cli.Command) error { called = true before, err := os.ReadFile(diagnostic.Name()) require.NoError(t, err) require.Empty(t, before, "preference warning must wait for successful presentation") - presentation := ctx.Value(imagePresentationKey{}).(imagePresentation) - require.Equal(t, "off", presentation.plan.inline) + require.Equal(t, "off", images.InlineForTest(ctx)) switch scenario { case "action failure": return failure @@ -165,9 +99,9 @@ func TestImageInlinePreferenceWarningTerminal(t *testing.T) { cancel() } return nil - }), + }, custom.ImageSavingOptions()), } - registerImageSavingFlags(command) + images.TestRegisterImageSavingFlags(command) err = command.Run(ctx, []string{"generate", "--output-dir", t.TempDir()}) require.True(t, called, "warning output must not prevent the action") switch scenario { @@ -196,8 +130,8 @@ func TestImagePreviewCommandsRegisterOnceAndRejectFormats(t *testing.T) { for _, argv := range [][]string{{"images", "preview"}, {"images", "preview", "one", "two"}, {"--format", "json", "images", "preview", "one"}, {"--transform", "id", "images", "inline", "on"}, {"--raw-output", "images", "inline", "off"}, {"images", "inline", "on", "extra"}} { t.Run(strings.Join(argv, " "), func(t *testing.T) { root := &cli.Command{Name: "openai", Writer: &bytes.Buffer{}, ErrWriter: &bytes.Buffer{}, Flags: []cli.Flag{&cli.StringFlag{Name: "format", Value: "auto"}, &cli.StringFlag{Name: "transform"}, &cli.BoolFlag{Name: "raw-output"}}, Commands: []*cli.Command{{Name: "images"}}} - ConfigureCommand(root) - ConfigureCommand(root) + custom.ConfigureCommand(root) + custom.ConfigureCommand(root) require.NotNil(t, root.Command("images").Command("preview")) require.NotNil(t, root.Command("images").Command("inline").Command("off")) require.Error(t, root.Run(t.Context(), append([]string{"openai"}, argv...))) @@ -220,13 +154,13 @@ func TestImagePreviewRegistrationPreservesInlineCommands(t *testing.T) { shared.Commands = append(shared.Commands, &cli.Command{Name: "existing"}) } if order == "saved first" { - ConfigureCommand(root) + custom.ConfigureCommand(root) addOtherCommands() } else { addOtherCommands() - ConfigureCommand(root) + custom.ConfigureCommand(root) } - ConfigureCommand(root) + custom.ConfigureCommand(root) require.Same(t, shared, images.Command("inline")) counts := map[string]int{} for _, command := range images.Commands { diff --git a/internal/commands/images/image_preferences_test.go b/internal/commands/images/image_preferences_test.go new file mode 100644 index 000000000..181881af4 --- /dev/null +++ b/internal/commands/images/image_preferences_test.go @@ -0,0 +1,78 @@ +package images + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/openai/openai-cli/internal/imageprefs" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v3" +) + +func TestImageInlinePreferencePrecedence(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("APPDATA", home) + t.Setenv("XDG_CONFIG_HOME", home) + path, err := imageInlinePreferencePath() + require.NoError(t, err) + for _, preset := range []string{"absent", "on", "off", "corrupt"} { + if preset != "absent" { + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0700)) + } + switch preset { + case "on", "off": + require.NoError(t, imageprefs.Save(t.Context(), path, preset == "on")) + case "corrupt": + require.NoError(t, os.WriteFile(path, []byte(`{"version":999,"inline":true}`), 0600)) + } + for _, flag := range []string{"", "auto", "on", "off"} { + for _, terminal := range []bool{false, true} { + name := preset + "/" + flag + t.Run(name, func(t *testing.T) { + app := &cli.Command{Name: "openai", Flags: []cli.Flag{&cli.StringFlag{Name: "inline", Value: "auto"}}, Action: func(ctx context.Context, command *cli.Command) error { + got, err := imageInlineMode(ctx, command, terminal) + if flag == "" && terminal && preset == "corrupt" { + require.Error(t, err) + return nil + } + require.NoError(t, err) + want := flag + if want == "" { + want = "auto" + if terminal && (preset == "on" || preset == "off") { + want = preset + } + } + require.Equal(t, want, got) + return nil + }} + args := []string{"openai"} + if flag != "" { + args = append(args, "--inline", flag) + } + require.NoError(t, app.Run(t.Context(), args)) + }) + } + } + } +} + +func TestImageInlinePreferenceLocationUsesSymbolicPaths(t *testing.T) { + for _, name := range []string{"HOME", "APPDATA", "USERPROFILE", "XDG_CONFIG_HOME"} { + t.Setenv(name, "synthetic-private-config") + } + for _, tc := range []struct{ goos, want string }{ + {"darwin", "$HOME/Library/Application Support/openai/image-preferences.json"}, + {"windows", `%AppData%\openai\image-preferences.json`}, + {"linux", "$XDG_CONFIG_HOME/openai/image-preferences.json, or $HOME/.config/openai/image-preferences.json when XDG_CONFIG_HOME is unset"}, + {"freebsd", "$XDG_CONFIG_HOME/openai/image-preferences.json, or $HOME/.config/openai/image-preferences.json when XDG_CONFIG_HOME is unset"}, + } { + t.Run(tc.goos, func(t *testing.T) { + require.Equal(t, tc.want, imageInlinePreferenceLocation(tc.goos)) + require.NotContains(t, imageInlinePreferenceLocation(tc.goos), "synthetic-private") + }) + } +} diff --git a/pkg/custom/image_saving_commands.go b/internal/commands/images/image_saving_commands.go similarity index 80% rename from pkg/custom/image_saving_commands.go rename to internal/commands/images/image_saving_commands.go index 186e32851..fdada0eb7 100644 --- a/pkg/custom/image_saving_commands.go +++ b/internal/commands/images/image_saving_commands.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -10,18 +10,19 @@ import ( "github.com/openai/openai-cli/internal/apiquery" clioutput "github.com/openai/openai-cli/internal/output" "github.com/openai/openai-cli/internal/readable" + requestconfig "github.com/openai/openai-cli/internal/request" "github.com/openai/openai-cli/internal/requestflag" + cliterminal "github.com/openai/openai-cli/internal/terminal" "github.com/openai/openai-go/v3/option" "github.com/tidwall/gjson" "github.com/urfave/cli/v3" ) -const imageGenerationOperation = "(resource) images > (method) generate" const imageSavingRequestMetadata = "openai-image-saving-request" -// Decorate the generated image commands with shared saving behavior. +// ConfigureSaving decorates the generated image commands with shared saving behavior. // Model discovery and terminal rendering have separate owners. -func configureImageSaving(root *cli.Command) { +func ConfigureSaving(root *cli.Command, opts SavingOptions) { images := root.Command("images") if images == nil { return @@ -42,7 +43,7 @@ func configureImageSaving(root *cli.Command) { } command.CustomHelpTemplate = imageUploadQuickHelp } - command.Action = imageSavingWorkflow(command.Action) + command.Action = imageSavingWorkflow(command.Action, opts) if name == "generate" { command.Action = imagePickerWorkflow(command.Action) } @@ -88,7 +89,7 @@ func registerImageSavingFlags(command *cli.Command) { type preparedImageSavingRequest struct { options []option.RequestOption consumed bool - bodyType BodyContentType + bodyType requestconfig.BodyContentType } type imagePresentationKey struct{} @@ -99,7 +100,7 @@ type imagePresentation struct { // Prepare once so stdin and @file values are not consumed twice. The generated // handler still owns SDK dispatch and response/stream lifetime. -func imageSavingWorkflow(next cli.ActionFunc) cli.ActionFunc { +func imageSavingWorkflow(next cli.ActionFunc, opts SavingOptions) cli.ActionFunc { return func(ctx context.Context, command *cli.Command) (resultErr error) { inline := command.String("inline") if inline != "auto" && inline != "on" && inline != "off" { @@ -118,12 +119,12 @@ func imageSavingWorkflow(next cli.ActionFunc) cli.ActionFunc { var streaming bool var warnPreference bool var err error - bodyType := ApplicationJSON + bodyType := requestconfig.ApplicationJSON if command.Name != "generate" { if command.Args().Len() != 0 { return imageSavingFailure("Unexpected extra arguments. Put source filenames after --image; use --help for usage.", nil) } - bodyType = MultipartFormEncoded + bodyType = requestconfig.MultipartFormEncoded state := &imageMultipartPreparation{context: ctx} // Prepared uploads still need an owner if the generated action fails // before dispatch. Close is idempotent after the SDK has used the body. @@ -134,11 +135,11 @@ func imageSavingWorkflow(next cli.ActionFunc) cli.ActionFunc { }() command.Metadata[imageMultipartMetadata] = state defer delete(command.Metadata, imageMultipartMetadata) - options, err = FlagOptions(command, apiquery.NestedQueryFormatBrackets, apiquery.ArrayQueryFormatBrackets, bodyType, false) + options, err = opts.RequestOptions(command, apiquery.NestedQueryFormatBrackets, apiquery.ArrayQueryFormatBrackets, bodyType, false) plan, streaming = state.plan, state.streaming } else { var body gjson.Result - options, err = FlagOptions(command, apiquery.NestedQueryFormatBrackets, apiquery.ArrayQueryFormatBrackets, bodyType, false, + options, err = opts.RequestOptions(command, apiquery.NestedQueryFormatBrackets, apiquery.ArrayQueryFormatBrackets, bodyType, false, func(raw []byte) { body = gjson.ParseBytes(raw) }) if err == nil { plan, streaming, err = prepareImageSaving(ctx, command, body) @@ -151,7 +152,7 @@ func imageSavingWorkflow(next cli.ActionFunc) cli.ActionFunc { return err } if plan != nil { - mode, preferenceErr := imageInlineMode(ctx, command, isTerminal(imageCommandWriter(command))) + mode, preferenceErr := imageInlineMode(ctx, command, cliterminal.IsWriterTerminal(imageCommandWriter(command))) if preferenceErr != nil { if ctx.Err() != nil { return ctx.Err() @@ -170,10 +171,10 @@ func imageSavingWorkflow(next cli.ActionFunc) cli.ActionFunc { command.Metadata[imageSavingRequestMetadata] = &preparedImageSavingRequest{options: options, bodyType: bodyType} defer delete(command.Metadata, imageSavingRequestMetadata) ctx = context.WithValue(ctx, imagePresentationKey{}, imagePresentation{plan, command.Root().Writer}) - if err := runWithImageLoading(ctx, command, plan, next); err != nil { + if err := runWithImageLoading(ctx, command, plan, next, opts); err != nil { return err } - if warnPreference && outputDiagnosticsAllowed(ctx) { + if warnPreference && opts.DiagnosticsAllowed(ctx) { // Wait until presentation succeeds so this plain-text warning cannot // mix with a structured command error. ErrWriter buffers failures. return readable.WriteText(clioutput.Writer{Context: ctx, Sink: os.Stderr}, "Could not read the inline preference; automatic previews are off for this command. Use --inline auto, on or off to override it, or check your image-preferences.json settings.") @@ -182,7 +183,8 @@ func imageSavingWorkflow(next cli.ActionFunc) cli.ActionFunc { } } -func consumeImageSavingRequest(command *cli.Command, nested apiquery.NestedQueryFormat, array apiquery.ArrayQueryFormat, body BodyContentType, ignoreStdin bool) ([]option.RequestOption, bool, error) { +// ConsumeSavingRequest returns one prepared request to its generated handler. +func ConsumeSavingRequest(command *cli.Command, nested apiquery.NestedQueryFormat, array apiquery.ArrayQueryFormat, body requestconfig.BodyContentType, ignoreStdin bool) ([]option.RequestOption, bool, error) { prepared, ok := command.Metadata[imageSavingRequestMetadata].(*preparedImageSavingRequest) if !ok { return nil, false, nil @@ -219,19 +221,23 @@ func selectImageGenerationStream(command *cli.Command, streaming bool) (func(), return nil, errors.New("image command has no compatible stream selector") } -func savedImagePresentation(opts ShowJSONOpts, kind OutputKind) (imagePresentation, bool) { - if opts.OutputKind != kind || opts.Context == nil { +func savedImagePresentation(ctx context.Context) (imagePresentation, bool) { + if ctx == nil { return imagePresentation{}, false } - switch opts.Operation { - case imageGenerationOperation, "(resource) images > (method) edit", "(resource) images > (method) create_variation": - default: - return imagePresentation{}, false - } - presentation, ok := opts.Context.Value(imagePresentationKey{}).(imagePresentation) + presentation, ok := ctx.Value(imagePresentationKey{}).(imagePresentation) return presentation, ok && presentation.plan != nil } +// ShowSavedResponse handles a response selected by application routing. +func ShowSavedResponse(ctx context.Context, res []byte) (bool, error) { + presentation, ok := savedImagePresentation(ctx) + if !ok { + return false, nil + } + return true, presentation.output(func(out io.Writer) error { return presentation.plan.save(ctx, res, out) }) +} + func (p imagePresentation) output(write func(io.Writer) error) error { out := p.writer if out == nil { diff --git a/pkg/custom/image_upload_help.go b/internal/commands/images/image_upload_help.go similarity index 99% rename from pkg/custom/image_upload_help.go rename to internal/commands/images/image_upload_help.go index 179585b6d..239942ae4 100644 --- a/pkg/custom/image_upload_help.go +++ b/internal/commands/images/image_upload_help.go @@ -1,4 +1,4 @@ -package custom +package images const imageUploadQuickHelp = `{{$bin := or (index .Root.Metadata "help-invocation") "openai"}}{{if eq .Name "edit"}}Edit an image {{$bin}} images edit --image "photo.png" --prompt "Make the sky purple" diff --git a/internal/commands/images/input_fixture_test.go b/internal/commands/images/input_fixture_test.go new file mode 100644 index 000000000..d254023ec --- /dev/null +++ b/internal/commands/images/input_fixture_test.go @@ -0,0 +1,62 @@ +package images + +import ( + "io" + "sync" + "sync/atomic" + "time" +) + +const multipartTestTimeout = 5 * time.Second + +type errorReader struct { + err error +} + +func (r errorReader) Read([]byte) (int, error) { + return 0, r.err +} + +type recordingReadCloser struct { + reader io.Reader + closeErr error + closeCount atomic.Int32 +} + +func (r *recordingReadCloser) Read(p []byte) (int, error) { + return r.reader.Read(p) +} + +func (r *recordingReadCloser) Close() error { + r.closeCount.Add(1) + return r.closeErr +} + +type closeUnblocksReader struct { + err error + readStarted chan struct{} + closed chan struct{} + startOnce sync.Once + closeOnce sync.Once + closeCount atomic.Int32 +} + +func newCloseUnblocksReader(err error) *closeUnblocksReader { + return &closeUnblocksReader{ + err: err, + readStarted: make(chan struct{}), + closed: make(chan struct{}), + } +} + +func (r *closeUnblocksReader) Read([]byte) (int, error) { + r.startOnce.Do(func() { close(r.readStarted) }) + <-r.closed + return 0, r.err +} + +func (r *closeUnblocksReader) Close() error { + r.closeCount.Add(1) + r.closeOnce.Do(func() { close(r.closed) }) + return nil +} diff --git a/pkg/custom/interactive_image_preview.go b/internal/commands/images/interactive_image_preview.go similarity index 96% rename from pkg/custom/interactive_image_preview.go rename to internal/commands/images/interactive_image_preview.go index 2c49c0ee7..a573c9448 100644 --- a/pkg/custom/interactive_image_preview.go +++ b/internal/commands/images/interactive_image_preview.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" @@ -71,13 +71,13 @@ func interactiveImageEligible(input, output bool, getenv func(string) string, go return err == nil && number >= 220400 } -func handleInteractiveImagePreview(parent context.Context, command *cli.Command) (resultErr error) { +func handleInteractiveImagePreview(parent context.Context, command *cli.Command, opts PreviewOptions) (resultErr error) { if command.IsSet("inline") { return imageSavingFailure("Use --interactive by itself; --inline selects a static preview.", nil) } input, _ := command.Root().Reader.(*os.File) output, _ := imageCommandWriter(command).(*os.File) - if !interactiveImageEligible(input != nil && isTerminal(input), output != nil && isTerminal(output), os.Getenv, runtime.GOOS) { + if !interactiveImageEligible(input != nil && cliterminal.IsWriterTerminal(input), output != nil && cliterminal.IsWriterTerminal(output), os.Getenv, runtime.GOOS) { return imageSavingFailure("Interactive previews require Konsole 22.04 or newer, terminal input and output, and a macOS or Linux CLI. Run outside CI and multiplexers.", nil) } path, err := filepath.Abs(command.Args().First()) @@ -110,7 +110,7 @@ func handleInteractiveImagePreview(parent context.Context, command *cli.Command) defer func() { resultErr = finishInteractiveImagePreview(ctx, errors.Join(resultErr, closeOutput())) if reporter != nil { - resultErr = reportInteractiveImageFailure(command, reporter, resultErr) + resultErr = reportInteractiveImageFailure(command, reporter, resultErr, opts) } }() executable, err := os.Executable() @@ -152,7 +152,7 @@ func finishInteractiveImagePreview(ctx context.Context, err error) error { return nil } -func reportInteractiveImageFailure(command *cli.Command, output *cliterminal.Output, failure error) (result error) { +func reportInteractiveImageFailure(command *cli.Command, output *cliterminal.Output, failure error, opts PreviewOptions) (result error) { defer func() { if err := output.Close(); err != nil { code := 1 @@ -175,7 +175,7 @@ func reportInteractiveImageFailure(command *cli.Command, output *cliterminal.Out code = exit.ExitCode() } var diagnostic bytes.Buffer - err := ShowCommandError(command.Root(), failure, &diagnostic) + err := opts.ShowError(command.Root(), failure, &diagnostic) if err == nil { ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) defer cancel() diff --git a/pkg/custom/interactive_image_preview_input.go b/internal/commands/images/interactive_image_preview_input.go similarity index 99% rename from pkg/custom/interactive_image_preview_input.go rename to internal/commands/images/interactive_image_preview_input.go index 42b8f32f5..0bed3b345 100644 --- a/pkg/custom/interactive_image_preview_input.go +++ b/internal/commands/images/interactive_image_preview_input.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" diff --git a/internal/commands/images/interactive_image_preview_integration_test.go b/internal/commands/images/interactive_image_preview_integration_test.go new file mode 100644 index 000000000..2c682f69b --- /dev/null +++ b/internal/commands/images/interactive_image_preview_integration_test.go @@ -0,0 +1,42 @@ +package images_test + +import ( + "bytes" + "context" + "errors" + "io" + "testing" + + "github.com/openai/openai-cli/internal/commands/images" + "github.com/openai/openai-cli/pkg/custom" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v3" +) + +func TestInteractiveImageFailureKeepsExitCodeAndViewerDiagnostic(t *testing.T) { + for _, code := range []int{0, 130, 143, 129} { + ctx, cancel := context.WithCancelCause(t.Context()) + cancel(images.InteractiveExitForTest(code)) + err := images.TestFinishInteractiveImagePreview(ctx, nil) + if code == 0 { + require.NoError(t, err) + } else { + var exit cli.ExitCoder + require.ErrorAs(t, err, &exit) + require.Equal(t, code, exit.ExitCode()) + require.Empty(t, exit.Error()) + } + failure := errors.Join(context.DeadlineExceeded, io.ErrClosedPipe) + err = images.TestFinishInteractiveImagePreview(ctx, failure) + var exit cli.ExitCoder + require.ErrorAs(t, err, &exit) + require.Equal(t, max(1, code), exit.ExitCode()) + require.ErrorIs(t, err, failure) + var output bytes.Buffer + require.NoError(t, custom.ShowCommandError(&cli.Command{}, err, &output)) + require.Contains(t, output.String(), "interactive preview") + require.Contains(t, output.String(), "original file is unchanged") + require.NotContains(t, output.String(), "API") + require.NotContains(t, output.String(), "permissions") + } +} diff --git a/pkg/custom/interactive_image_preview_test.go b/internal/commands/images/interactive_image_preview_test.go similarity index 87% rename from pkg/custom/interactive_image_preview_test.go rename to internal/commands/images/interactive_image_preview_test.go index 0e93ea067..cabee2fc3 100644 --- a/pkg/custom/interactive_image_preview_test.go +++ b/internal/commands/images/interactive_image_preview_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" @@ -16,7 +16,6 @@ import ( "time" "github.com/stretchr/testify/require" - "github.com/urfave/cli/v3" ) func TestInteractiveImageEligibility(t *testing.T) { @@ -215,31 +214,3 @@ func TestInteractiveImageCancellationPreservesOtherFailures(t *testing.T) { require.ErrorIs(t, err, io.ErrUnexpectedEOF) require.NotErrorIs(t, err, context.Canceled) } - -func TestInteractiveImageFailureKeepsExitCodeAndViewerDiagnostic(t *testing.T) { - for _, code := range []int{0, 130, 143, 129} { - ctx, cancel := context.WithCancelCause(t.Context()) - cancel(interactiveImageExit{code}) - err := finishInteractiveImagePreview(ctx, nil) - if code == 0 { - require.NoError(t, err) - } else { - var exit cli.ExitCoder - require.ErrorAs(t, err, &exit) - require.Equal(t, code, exit.ExitCode()) - require.Empty(t, exit.Error()) - } - failure := errors.Join(context.DeadlineExceeded, io.ErrClosedPipe) - err = finishInteractiveImagePreview(ctx, failure) - var exit cli.ExitCoder - require.ErrorAs(t, err, &exit) - require.Equal(t, max(1, code), exit.ExitCode()) - require.ErrorIs(t, err, failure) - var output bytes.Buffer - require.NoError(t, ShowCommandError(&cli.Command{}, err, &output)) - require.Contains(t, output.String(), "interactive preview") - require.Contains(t, output.String(), "original file is unchanged") - require.NotContains(t, output.String(), "API") - require.NotContains(t, output.String(), "permissions") - } -} diff --git a/internal/commands/images/options.go b/internal/commands/images/options.go new file mode 100644 index 000000000..1398200ae --- /dev/null +++ b/internal/commands/images/options.go @@ -0,0 +1,38 @@ +// Package images owns handwritten image workflows, saving, previews, and picker state. +package images + +import ( + "context" + "io" + + "github.com/openai/openai-cli/internal/apiquery" + requestconfig "github.com/openai/openai-cli/internal/request" + "github.com/openai/openai-go/v3/option" + "github.com/tidwall/gjson" + "github.com/urfave/cli/v3" +) + +// RequestOptions prepares a generated request using assembly-owned hooks. +type RequestOptions func(*cli.Command, apiquery.NestedQueryFormat, apiquery.ArrayQueryFormat, requestconfig.BodyContentType, bool, ...func([]byte)) ([]option.RequestOption, error) + +// ModelOptions connects image model discovery to application request and output policy. +type ModelOptions struct { + RequestOptions RequestOptions + DefaultRequestOptions func(*cli.Command) []option.RequestOption + HumanOutput func(*cli.Command) bool + DiagnosticsAllowed func(context.Context) bool + ShowData func(context.Context, *cli.Command, gjson.Result, io.Writer) error +} + +// SavingOptions supplies policy while the feature owns preparation and image lifetime. +type SavingOptions struct { + RequestOptions RequestOptions + DiagnosticsAllowed func(context.Context) bool + ErrorFormat func(*cli.Command) string +} + +// PreviewOptions connects local previews to receipt and error presentation. +type PreviewOptions struct { + Receipt func(context.Context, string) error + ShowError func(*cli.Command, error, io.Writer) error +} diff --git a/internal/commands/images/options_integration_test.go b/internal/commands/images/options_integration_test.go new file mode 100644 index 000000000..3d1e1a1dd --- /dev/null +++ b/internal/commands/images/options_integration_test.go @@ -0,0 +1,25 @@ +package images_test + +import ( + "context" + "strings" + + "github.com/openai/openai-cli/pkg/custom" + "github.com/urfave/cli/v3" +) + +func imageErrorMessage(command *cli.Command, failure error) string { + if command == nil { + command = &cli.Command{} + } + var out strings.Builder + if err := custom.ShowCommandError(command, failure, &out); err != nil { + panic(err) + } + return strings.TrimSpace(out.String()) +} + +func imageWrapError(command *cli.Command, failure error) error { + custom.ConfigureCommandErrors(command) + return command.OnUsageError(context.Background(), command, failure, false) +} diff --git a/pkg/custom/prepare_image_upload.go b/internal/commands/images/prepare_image_upload.go similarity index 86% rename from pkg/custom/prepare_image_upload.go rename to internal/commands/images/prepare_image_upload.go index 909a7eb89..783241c06 100644 --- a/pkg/custom/prepare_image_upload.go +++ b/internal/commands/images/prepare_image_upload.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -20,10 +20,10 @@ type imageMultipartPreparation struct { body io.Closer } -// Inspect merged scalar settings after trusted file expansion, before the +// PrepareMultipartBody inspects merged scalar settings after trusted file expansion, before the // existing multipart encoder takes ownership. Source images and masks remain // streamed and are never decoded, buffered, or rewritten here. -func prepareImageMultipartBody(command *cli.Command, body map[string]any) error { +func PrepareMultipartBody(command *cli.Command, body map[string]any) error { state, ok := command.Metadata[imageMultipartMetadata].(*imageMultipartPreparation) if !ok { return nil @@ -86,3 +86,10 @@ func inspectImageMultipartScalar(name, text string) any { } return text } + +// ObserveMultipart retains the encoded body for cleanup if dispatch fails early. +func ObserveMultipart(command *cli.Command, body io.Closer) { + if state, ok := command.Metadata[imageMultipartMetadata].(*imageMultipartPreparation); ok { + state.body = body + } +} diff --git a/internal/commands/images/prepare_image_upload_integration_test.go b/internal/commands/images/prepare_image_upload_integration_test.go new file mode 100644 index 000000000..3183cac11 --- /dev/null +++ b/internal/commands/images/prepare_image_upload_integration_test.go @@ -0,0 +1,52 @@ +package images_test + +import ( + "context" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/openai/openai-cli/internal/commands/images" + "github.com/openai/openai-cli/internal/requestflag" + "github.com/openai/openai-cli/pkg/custom" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v3" +) + +func TestImageUploadPreparationClosesWhenActionAborts(t *testing.T) { + for _, cancelAction := range []bool{false, true} { + t.Run(fmt.Sprint(cancelAction), func(t *testing.T) { + source := filepath.Join(t.TempDir(), "source.png") + require.NoError(t, os.WriteFile(source, []byte("synthetic-source"), 0600)) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + failure := errors.New("synthetic action failure") + var prepared io.ReadCloser + command := &cli.Command{Name: "edit", Reader: strings.NewReader(""), Writer: io.Discard, + Flags: []cli.Flag{&requestflag.Flag[string]{Name: "image", BodyPath: "image", FileInput: true}}, + Action: images.TestImageSavingWorkflow(func(ctx context.Context, command *cli.Command) error { + prepared = images.PreparedBodyForTest(command) + if cancelAction { + cancel() + return ctx.Err() + } + return failure + }, custom.ImageSavingOptions()), + } + images.TestRegisterImageSavingFlags(command) + err := command.Run(ctx, []string{"edit", "--image", source, "--output-dir", t.TempDir()}) + if cancelAction { + require.ErrorIs(t, err, context.Canceled) + } else { + require.ErrorIs(t, err, failure) + } + require.NotNil(t, prepared) + _, err = io.ReadAll(prepared) + require.ErrorIs(t, err, io.ErrClosedPipe) + }) + } +} diff --git a/pkg/custom/prepare_image_upload_test.go b/internal/commands/images/prepare_image_upload_test.go similarity index 75% rename from pkg/custom/prepare_image_upload_test.go rename to internal/commands/images/prepare_image_upload_test.go index 4e3331330..89cead1a3 100644 --- a/pkg/custom/prepare_image_upload_test.go +++ b/internal/commands/images/prepare_image_upload_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -6,17 +6,13 @@ import ( "errors" "fmt" "io" - "os" - "path/filepath" "strings" "testing" "time" "github.com/openai/openai-cli/internal/apiform" requestconfig "github.com/openai/openai-cli/internal/request" - "github.com/openai/openai-cli/internal/requestflag" "github.com/stretchr/testify/require" - "github.com/urfave/cli/v3" ) func TestImageMultipartScalarInspectionPreservesText(t *testing.T) { @@ -112,41 +108,6 @@ func TestImageMultipartScalarInspectionCancellationReleasesOwnedRead(t *testing. } } -func TestImageUploadPreparationClosesWhenActionAborts(t *testing.T) { - for _, cancelAction := range []bool{false, true} { - t.Run(fmt.Sprint(cancelAction), func(t *testing.T) { - source := filepath.Join(t.TempDir(), "source.png") - require.NoError(t, os.WriteFile(source, []byte("synthetic-source"), 0600)) - ctx, cancel := context.WithCancel(t.Context()) - defer cancel() - failure := errors.New("synthetic action failure") - var prepared io.ReadCloser - command := &cli.Command{Name: "edit", Reader: strings.NewReader(""), Writer: io.Discard, - Flags: []cli.Flag{&requestflag.Flag[string]{Name: "image", BodyPath: "image", FileInput: true}}, - Action: imageSavingWorkflow(func(ctx context.Context, command *cli.Command) error { - state := command.Metadata[imageMultipartMetadata].(*imageMultipartPreparation) - prepared = state.body.(io.ReadCloser) - if cancelAction { - cancel() - return ctx.Err() - } - return failure - }), - } - registerImageSavingFlags(command) - err := command.Run(ctx, []string{"edit", "--image", source, "--output-dir", t.TempDir()}) - if cancelAction { - require.ErrorIs(t, err, context.Canceled) - } else { - require.ErrorIs(t, err, failure) - } - require.NotNil(t, prepared) - _, err = io.ReadAll(prepared) - require.ErrorIs(t, err, io.ErrClosedPipe) - }) - } -} - func TestImageUploadPreparedBodyClosesOnce(t *testing.T) { source := &recordingReadCloser{reader: strings.NewReader("synthetic image")} var body io.Closer diff --git a/pkg/custom/prepare_native_image_output.go b/internal/commands/images/prepare_native_image_output.go similarity index 85% rename from pkg/custom/prepare_native_image_output.go rename to internal/commands/images/prepare_native_image_output.go index 6a706eb2a..8a258d2ad 100644 --- a/pkg/custom/prepare_native_image_output.go +++ b/internal/commands/images/prepare_native_image_output.go @@ -1,10 +1,11 @@ -package custom +package images import ( "context" "os" "strings" + cliterminal "github.com/openai/openai-cli/internal/terminal" "github.com/openai/openai-cli/internal/terminalimage" "github.com/urfave/cli/v3" ) @@ -22,7 +23,7 @@ func prepareNativeImageOutput(ctx context.Context, command *cli.Command) (contex return skip() } out := imageCommandWriter(command) - protocol := savedImageProtocol(command.String("inline"), isTerminal(out), os.Getenv) + protocol := savedImageProtocol(command.String("inline"), cliterminal.IsWriterTerminal(out), os.Getenv) if protocol != "kitty" && protocol != "iterm-auto" && protocol != "iterm-konsole" { return skip() } diff --git a/internal/commands/images/presentation_fixture_integration_test.go b/internal/commands/images/presentation_fixture_integration_test.go new file mode 100644 index 000000000..c15fa7525 --- /dev/null +++ b/internal/commands/images/presentation_fixture_integration_test.go @@ -0,0 +1,44 @@ +package images_test + +import ( + "context" + "io" + "testing" + + "github.com/openai/openai-cli/internal/output/transform" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v3" +) + +const OutputResponse = transform.OutputResponse +const OutputStreamEvent = transform.OutputStreamEvent + +type errorSink struct{ err error } + +func (e errorSink) Write([]byte) (int, error) { return 0, e.err } +func readableErrorTestCommand(t *testing.T, args ...string) *cli.Command { + t.Helper() + root := &cli.Command{ + Name: "openai", Writer: io.Discard, ErrWriter: io.Discard, HideHelpCommand: true, + Metadata: map[string]any{"help-invocation": "./openai"}, + Flags: []cli.Flag{ + &cli.StringFlag{Name: "format", Value: "auto"}, + &cli.StringFlag{Name: "format-error", Value: "auto"}, + &cli.StringFlag{Name: "transform-error"}, + &cli.StringFlag{Name: "transform"}, + &cli.BoolFlag{Name: "raw-output"}, + }, + Action: func(context.Context, *cli.Command) error { return nil }, + } + require.NoError(t, root.Run(t.Context(), append([]string{"openai"}, args...))) + return root +} + +func assertReadableErrorContainsNoPrivateDetails(t *testing.T, text string) { + t.Helper() + for _, private := range []string{"synthetic-", "https://", "token=", "\x1b", "\a", "\u202e"} { + require.NotContains(t, text, private) + } +} + +const imageGenerationOperation = "(resource) images > (method) generate" diff --git a/internal/commands/images/presentation_fixture_test.go b/internal/commands/images/presentation_fixture_test.go new file mode 100644 index 000000000..f33508b8f --- /dev/null +++ b/internal/commands/images/presentation_fixture_test.go @@ -0,0 +1,10 @@ +package images + +import ( + clioutput "github.com/openai/openai-cli/internal/output" + "github.com/openai/openai-cli/internal/terminal" +) + +type outputJSON = clioutput.JSON + +var isTerminal = terminal.IsWriterTerminal diff --git a/pkg/custom/preview_saved_image.go b/internal/commands/images/preview_saved_image.go similarity index 79% rename from pkg/custom/preview_saved_image.go rename to internal/commands/images/preview_saved_image.go index 13befad40..272551046 100644 --- a/pkg/custom/preview_saved_image.go +++ b/internal/commands/images/preview_saved_image.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" @@ -10,11 +10,13 @@ import ( "path/filepath" "strings" + cliterminal "github.com/openai/openai-cli/internal/terminal" "github.com/openai/openai-cli/internal/terminalimage" "github.com/urfave/cli/v3" ) -const imagePreviewDetails = `PNG, JPEG and WebP. No API call or key is needed. The original is unchanged. +// PreviewDetails is shared by preview command and onboarding help. +const PreviewDetails = `PNG, JPEG and WebP. No API call or key is needed. The original is unchanged. Preview size uses the current terminal window. Resize, then run it again. Automatic preview preferences do not affect this command. Use --interactive in Konsole for a live view that redraws after resizing. @@ -30,28 +32,29 @@ No API call or key needed. Keeps your original. Command help: {{$bin}} images preview --help ` -func registerImagePreviewCommands(root *cli.Command) { +// RegisterPreview installs local previews and inline preference commands. +func RegisterPreview(root *cli.Command, opts PreviewOptions) { images := root.Command("images") if images == nil { return } images.Commands = append(images.Commands, - &cli.Command{Name: "preview", Usage: "Display a saved image without generating another one.", UsageText: "openai images preview [--inline auto|on | --interactive] FILE", Description: imagePreviewDetails + "\nPipes, CI and terminals without supported graphics or color cannot display previews.\nPreview limits: 64 MiB and 16 megapixels. Larger originals are still kept.", CustomHelpTemplate: imagePreviewHelp, - Flags: []cli.Flag{&cli.StringFlag{Name: "inline", Value: "auto", Usage: "Preview using auto or on; on permits a local Apple Terminal image font"}, &cli.BoolFlag{Name: "interactive", Usage: "Open a live Konsole viewer on macOS or Linux; q or Escape closes it"}}, Action: handleImagesPreview}, + &cli.Command{Name: "preview", Usage: "Display a saved image without generating another one.", UsageText: "openai images preview [--inline auto|on | --interactive] FILE", Description: PreviewDetails + "\nPipes, CI and terminals without supported graphics or color cannot display previews.\nPreview limits: 64 MiB and 16 megapixels. Larger originals are still kept.", CustomHelpTemplate: imagePreviewHelp, + Flags: []cli.Flag{&cli.StringFlag{Name: "inline", Value: "auto", Usage: "Preview using auto or on; on permits a local Apple Terminal image font"}, &cli.BoolFlag{Name: "interactive", Usage: "Open a live Konsole viewer on macOS or Linux; q or Escape closes it"}}, Action: func(ctx context.Context, command *cli.Command) error { return handleImagesPreview(ctx, command, opts) }}, ) inline := images.Command("inline") if inline == nil { inline = &cli.Command{Name: "inline", Usage: "Turn automatic image previews on or off."} images.Commands = append(images.Commands, inline) } - for _, command := range imageInlinePreferenceCommands() { + for _, command := range imageInlinePreferenceCommands(opts.Receipt) { if inline.Command(command.Name) == nil { inline.Commands = append(inline.Commands, command) } } } -func handleImagesPreview(ctx context.Context, command *cli.Command) (resultErr error) { +func handleImagesPreview(ctx context.Context, command *cli.Command, opts PreviewOptions) (resultErr error) { if command.Args().Len() != 1 { return imageSavingFailure("Provide one saved image: openai images preview \"path/to/image.png\". Quote filenames containing spaces.", nil) } @@ -63,11 +66,11 @@ func handleImagesPreview(ctx context.Context, command *cli.Command) (resultErr e return imageSavingFailure("Use --inline auto or on with images preview.", nil) } out := imageCommandWriter(command) - if !isTerminal(out) { + if !cliterminal.IsWriterTerminal(out) { return imageSavingFailure("Image previews require a terminal. Run without piping or redirecting stdout.", nil) } if command.Bool("interactive") { - return handleInteractiveImagePreview(ctx, command) + return handleInteractiveImagePreview(ctx, command, opts) } protocol := savedImageProtocol(mode, true, os.Getenv) if protocol == "" { diff --git a/pkg/custom/save_final_image_stream.go b/internal/commands/images/save_final_image_stream.go similarity index 76% rename from pkg/custom/save_final_image_stream.go rename to internal/commands/images/save_final_image_stream.go index bf77abfb4..4591f6aba 100644 --- a/pkg/custom/save_final_image_stream.go +++ b/internal/commands/images/save_final_image_stream.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -9,12 +9,13 @@ import ( "github.com/openai/openai-cli/internal/jsonview" transformers "github.com/openai/openai-cli/internal/output/transform" + cliterminal "github.com/openai/openai-cli/internal/terminal" "github.com/tidwall/gjson" ) // Close the SDK stream even when stopping at completion before EOF. Never save // intermediate images to the output folder or wait for a redundant completion. -func saveFinalImageStream[T any](ctx context.Context, source jsonview.Iterator[T], plan *imageOutputPlan, out io.Writer) error { +func saveFinalImageStream[T any](ctx context.Context, source jsonview.Iterator[T], plan *imageOutputPlan, out io.Writer, quiet bool) error { if closer, ok := any(source).(io.Closer); ok { defer closer.Close() } @@ -29,7 +30,7 @@ func saveFinalImageStream[T any](ctx context.Context, source jsonview.Iterator[T } item := source.Current() var raw string - if value, ok := any(item).(hasRawJSON); ok { + if value, ok := any(item).(interface{ RawJSON() string }); ok { raw = value.RawJSON() } else { encoded, err := json.Marshal(item) @@ -42,12 +43,12 @@ func saveFinalImageStream[T any](ctx context.Context, source jsonview.Iterator[T switch event.Get("type").String() { case "image_generation.partial_image", "image_edit.partial_image": index := event.Get("partial_image_index") - if quietOutput(ctx) || progressUnavailable || plan.partialImages < 1 || plan.partialImages > 3 || + if quiet || progressUnavailable || plan.partialImages < 1 || plan.partialImages > 3 || index.Type != gjson.Number || index.Float() != float64(index.Int()) || index.Int() < 0 || index.Int() >= plan.partialImages || seen[index.Int()] { continue } - protocol := imageProgressProtocol(plan.inline, isTerminal(out), os.Getenv) + protocol := imageProgressProtocol(plan.inline, cliterminal.IsWriterTerminal(out), os.Getenv) if protocol == "" { continue } @@ -79,3 +80,12 @@ func saveFinalImageStream[T any](ctx context.Context, source jsonview.Iterator[T } return imageSavingFailure("The image stream ended without a final image. Check API usage before trying again.", nil) } + +// ShowSavedStream handles a stream selected by application routing, retaining stream cleanup ownership. +func ShowSavedStream[T any](ctx context.Context, source jsonview.Iterator[T], quiet bool) (bool, error) { + presentation, ok := savedImagePresentation(ctx) + if !ok { + return false, nil + } + return true, presentation.output(func(out io.Writer) error { return saveFinalImageStream(ctx, source, presentation.plan, out, quiet) }) +} diff --git a/internal/commands/images/save_final_image_stream_integration_test.go b/internal/commands/images/save_final_image_stream_integration_test.go new file mode 100644 index 000000000..2855df142 --- /dev/null +++ b/internal/commands/images/save_final_image_stream_integration_test.go @@ -0,0 +1,27 @@ +package images_test + +import ( + "errors" + "os" + "testing" + + "github.com/openai/openai-cli/internal/commands/images" + "github.com/stretchr/testify/require" +) + +func TestSavedImageOutputFailureRetainsFilesAndPartialError(t *testing.T) { + plan := images.NewOutputPlanForTest(t.TempDir(), "robot", "") + sinkErr := errors.New("closed synthetic sink") + err := plan.SaveForTest(t.Context(), []byte(`{"data":[{"b64_json":"iVBORw0KGgo="},{"b64_json":"invalid"}]}`), errorSink{sinkErr}) + require.ErrorIs(t, err, sinkErr) + require.Contains(t, err.Error(), "could not save the entire response") + require.Contains(t, err.Error(), "print all saved paths") + message := imageErrorMessage(nil, err) + require.Contains(t, message, "could not save the entire response") + require.Contains(t, message, "print all saved paths") + require.Contains(t, message, "Check the output folder before generating again") + require.NotContains(t, message, "listed files") + files, readErr := os.ReadDir(plan.DirectoryForTest()) + require.NoError(t, readErr) + require.Len(t, files, 1) +} diff --git a/pkg/custom/save_final_image_stream_test.go b/internal/commands/images/save_final_image_stream_test.go similarity index 58% rename from pkg/custom/save_final_image_stream_test.go rename to internal/commands/images/save_final_image_stream_test.go index 38f5d532d..20185b249 100644 --- a/pkg/custom/save_final_image_stream_test.go +++ b/internal/commands/images/save_final_image_stream_test.go @@ -1,4 +1,4 @@ -package custom +package images import ( "bytes" @@ -26,13 +26,13 @@ func (s *savedImageTestStream) Close() error { s.closed = true; return ni func TestSaveFinalImageStreamStopsAndClosesAtCompletion(t *testing.T) { stream := &savedImageTestStream{events: []outputJSON{ - {gjson.Parse(`{"type":"image_generation.partial_image","b64_json":"ignored"}`)}, - {gjson.Parse(`{"type":"image_generation.completed","b64_json":"iVBORw0KGgo="}`)}, - {gjson.Parse(`{"type":"image_generation.failed"}`)}, + {Result: gjson.Parse(`{"type":"image_generation.partial_image","b64_json":"ignored"}`)}, + {Result: gjson.Parse(`{"type":"image_generation.completed","b64_json":"iVBORw0KGgo="}`)}, + {Result: gjson.Parse(`{"type":"image_generation.failed"}`)}, }} var output bytes.Buffer plan := &imageOutputPlan{directory: t.TempDir(), name: "robot"} - require.NoError(t, saveFinalImageStream(t.Context(), stream, plan, &output)) + require.NoError(t, saveFinalImageStream(t.Context(), stream, plan, &output, false)) require.Equal(t, 2, stream.index) require.True(t, stream.closed) files, err := os.ReadDir(plan.directory) @@ -49,12 +49,12 @@ func TestSaveFinalImageStreamFailureAndCancellation(t *testing.T) { }{ {name: "EOF"}, {name: "transport", sourceErr: errors.New("synthetic-private-response")}, - {name: "malformed final", events: []outputJSON{{gjson.Parse(`{"type":"image_generation.completed","b64_json":{"secret":"synthetic-private-response"}}`)}}}, - {name: "failure event", events: []outputJSON{{gjson.Parse(`{"type":"error","message":"synthetic-private-response"}`)}}}, + {name: "malformed final", events: []outputJSON{{Result: gjson.Parse(`{"type":"image_generation.completed","b64_json":{"secret":"synthetic-private-response"}}`)}}}, + {name: "failure event", events: []outputJSON{{Result: gjson.Parse(`{"type":"error","message":"synthetic-private-response"}`)}}}, } { t.Run(test.name, func(t *testing.T) { stream := &savedImageTestStream{events: test.events, err: test.sourceErr} - err := saveFinalImageStream(t.Context(), stream, &imageOutputPlan{directory: t.TempDir()}, io.Discard) + err := saveFinalImageStream(t.Context(), stream, &imageOutputPlan{directory: t.TempDir()}, io.Discard, false) require.Error(t, err) require.NotContains(t, err.Error(), "synthetic-private-response") require.Contains(t, err.Error(), "before trying again") @@ -67,7 +67,7 @@ func TestSaveFinalImageStreamFailureAndCancellation(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) cancel() stream := &savedImageTestStream{} - require.ErrorIs(t, saveFinalImageStream(ctx, stream, &imageOutputPlan{}, io.Discard), context.Canceled) + require.ErrorIs(t, saveFinalImageStream(ctx, stream, &imageOutputPlan{}, io.Discard, false), context.Canceled) require.Zero(t, stream.index) require.True(t, stream.closed) } @@ -75,20 +75,3 @@ func TestSaveFinalImageStreamFailureAndCancellation(t *testing.T) { type failingImageWriter struct{ err error } func (w failingImageWriter) Write([]byte) (int, error) { return 0, w.err } - -func TestSavedImageOutputFailureRetainsFilesAndPartialError(t *testing.T) { - plan := &imageOutputPlan{directory: t.TempDir(), name: "robot"} - sinkErr := errors.New("closed synthetic sink") - err := plan.save(t.Context(), []byte(`{"data":[{"b64_json":"iVBORw0KGgo="},{"b64_json":"invalid"}]}`), failingImageWriter{sinkErr}) - require.ErrorIs(t, err, sinkErr) - require.Contains(t, err.Error(), "could not save the entire response") - require.Contains(t, err.Error(), "print all saved paths") - message := localErrorMessage(nil, err) - require.Contains(t, message, "could not save the entire response") - require.Contains(t, message, "print all saved paths") - require.Contains(t, message, "Check the output folder before generating again") - require.NotContains(t, message, "listed files") - files, readErr := os.ReadDir(plan.directory) - require.NoError(t, readErr) - require.Len(t, files, 1) -} diff --git a/pkg/custom/save_generated_image_identity_test.go b/internal/commands/images/save_generated_image_identity_test.go similarity index 99% rename from pkg/custom/save_generated_image_identity_test.go rename to internal/commands/images/save_generated_image_identity_test.go index 1c5e8df74..38afdea9a 100644 --- a/pkg/custom/save_generated_image_identity_test.go +++ b/internal/commands/images/save_generated_image_identity_test.go @@ -1,6 +1,6 @@ //go:build unix -package custom +package images import ( "bytes" diff --git a/pkg/custom/save_generated_images.go b/internal/commands/images/save_generated_images.go similarity index 93% rename from pkg/custom/save_generated_images.go rename to internal/commands/images/save_generated_images.go index 1c4f2d8f9..c663b90f3 100644 --- a/pkg/custom/save_generated_images.go +++ b/internal/commands/images/save_generated_images.go @@ -1,4 +1,4 @@ -package custom +package images import ( "context" @@ -57,26 +57,26 @@ type imageOutputPlan struct { partialImages int64 stopLoading func() loadingPrompt string - loadingStage func(imageLoadingStage) + loadingStage func(clioutput.LoadingStage) } // A private classification carries locally authored guidance through the shared // error presenter. Causes remain available to errors.Is without printing data. -type imageSavingError struct { +type SavingError struct { message string cause error preserveOnCancel bool } -func (e *imageSavingError) Error() string { return e.message } -func (e *imageSavingError) Unwrap() error { return e.cause } +func (e *SavingError) Error() string { return e.message } +func (e *SavingError) Unwrap() error { return e.cause } func imageSavingFailure(message string, cause error) error { - return &imageSavingError{message: message, cause: cause} + return &SavingError{message: message, cause: cause} } // Only path-free saving outcomes opt in; other image errors can include private paths. func imageSavingResultFailure(message string, cause error) error { - return &imageSavingError{message: message, cause: cause, preserveOnCancel: true} + return &SavingError{message: message, cause: cause, preserveOnCancel: true} } func prepareImageSaving(ctx context.Context, command *cli.Command, body gjson.Result) (*imageOutputPlan, bool, error) { @@ -184,10 +184,10 @@ func prepareImageSaving(ctx context.Context, command *cli.Command, body gjson.Re } func (p *imageOutputPlan) save(ctx context.Context, response []byte, out io.Writer) error { - p.setLoadingStage(imageLoadingSaving) + p.setLoadingStage(clioutput.LoadingSaving) saved, saveErr := imageoutput.SaveResponse(ctx, response, p.directory, p.name) if saveErr == nil && len(saved) > 0 { - p.setLoadingStage(imageLoadingSaved) + p.setLoadingStage(clioutput.LoadingSaved) } p.stopLoadingFeedback() previewOutput := out @@ -226,3 +226,6 @@ func (p *imageOutputPlan) save(ctx context.Context, response []byte, out io.Writ } return nil } + +// PreserveOnCancel retains guidance about files already saved before cancellation. +func (e *SavingError) PreserveOnCancel() bool { return e.preserveOnCancel } diff --git a/pkg/custom/save_generated_images_cancellation_test.go b/internal/commands/images/save_generated_images_cancellation_test.go similarity index 83% rename from pkg/custom/save_generated_images_cancellation_test.go rename to internal/commands/images/save_generated_images_cancellation_test.go index 8862e7626..591867ba2 100644 --- a/pkg/custom/save_generated_images_cancellation_test.go +++ b/internal/commands/images/save_generated_images_cancellation_test.go @@ -1,4 +1,4 @@ -package custom +package images_test import ( "bytes" @@ -15,6 +15,9 @@ import ( "testing" "github.com/goccy/go-yaml" + "github.com/openai/openai-cli/internal/commands/images" + clioutput "github.com/openai/openai-cli/internal/output" + "github.com/openai/openai-cli/pkg/custom" "github.com/stretchr/testify/require" "github.com/tidwall/gjson" "github.com/urfave/cli/v3" @@ -60,19 +63,19 @@ func TestSavedImageCancellationPreservesRecovery(t *testing.T) { var receipt bytes.Buffer receiptCalls := 0 sinkErr := errors.New("synthetic-private-sink \x1b]52;c;synthetic-secret\a") - writer := loadingTestWriter(func(data []byte) (int, error) { + writer := images.TestLoadingTestWriter(func(data []byte) (int, error) { receiptCalls++ require.ErrorIs(t, ctx.Err(), context.Canceled, "completed paths must print after cancellation") _, err := os.Stat(second) require.ErrorIs(t, err, os.ErrNotExist, "incomplete output must be removed before its receipt") if failReceipt { - return failingImageWriter{sinkErr}.Write(data) + return errorSink{sinkErr}.Write(data) } return receipt.Write(data) }) - plan := &imageOutputPlan{directory: directory, name: "robot", inline: "off"} - presentation := context.WithValue(checking, imagePresentationKey{}, imagePresentation{plan, writer}) - failure := ShowJSON(response, ShowJSONOpts{ + plan := images.NewOutputPlanForTest(directory, "robot", "off") + presentation := images.WithPresentationForTest(checking, plan, writer) + failure := custom.ShowJSON(response, custom.ShowJSONOpts{ Context: presentation, Operation: imageGenerationOperation, OutputKind: OutputResponse, Format: "auto", Stdout: writer, Stderr: io.Discard, }) @@ -98,12 +101,12 @@ func TestSavedImageCancellationPreservesRecovery(t *testing.T) { root := readableErrorTestCommand(t, "--format-error", format) for _, interrupted := range []bool{false, true} { wrapped := failure - wantStatus := 1 // main's fallback; no native process execution is claimed here. + wantStatus := 1 if interrupted { - wrapped = &imageLoadingInterrupt{failure} + wrapped = images.InterruptForTest(failure) wantStatus = 130 } - wrapped = withCommandError(root, wrapped) + wrapped = imageWrapError(root, wrapped) require.ErrorIs(t, wrapped, context.Canceled) if failReceipt { require.ErrorIs(t, wrapped, sinkErr) @@ -115,7 +118,7 @@ func TestSavedImageCancellationPreservesRecovery(t *testing.T) { } require.Equal(t, wantStatus, status) var diagnostic bytes.Buffer - require.NoError(t, ShowCommandError(root, wrapped, &diagnostic)) + require.NoError(t, custom.ShowCommandError(root, wrapped, &diagnostic)) assertReadableErrorContainsNoPrivateDetails(t, diagnostic.String()) require.NotContains(t, diagnostic.String(), encoded) message := diagnostic.String() @@ -140,16 +143,6 @@ func TestSavedImageCancellationPreservesRecovery(t *testing.T) { } } -type imageSaveCancellationContext struct { - context.Context - check func() -} - -func (c imageSaveCancellationContext) Err() error { - c.check() - return c.Context.Err() -} - func TestSavedImageCancellationWithoutCompletedFiles(t *testing.T) { var fixture bytes.Buffer require.NoError(t, png.Encode(&fixture, image.NewRGBA(image.Rect(0, 0, 1, 1)))) @@ -181,21 +174,21 @@ func TestSavedImageCancellationWithoutCompletedFiles(t *testing.T) { boundaryReached = true cancel() }} - plan := &imageOutputPlan{directory: directory, name: "synthetic-private-image", inline: "off"} - plan.loadingStage = func(stage imageLoadingStage) { - if boundary == "before_first_save" && stage == imageLoadingSaving { + plan := images.NewOutputPlanForTest(directory, "synthetic-private-image", "off") + plan.SetLoadingStageForTest(func(stage clioutput.LoadingStage) { + if boundary == "before_first_save" && stage == clioutput.LoadingSaving { boundaryReached = true cancel() } - } + }) var receipt bytes.Buffer receiptCalls := 0 - writer := loadingTestWriter(func(data []byte) (int, error) { + writer := images.TestLoadingTestWriter(func(data []byte) (int, error) { receiptCalls++ return receipt.Write(data) }) - presentation := context.WithValue(checking, imagePresentationKey{}, imagePresentation{plan, writer}) - failure := ShowJSON(response, ShowJSONOpts{ + presentation := images.WithPresentationForTest(checking, plan, writer) + failure := custom.ShowJSON(response, custom.ShowJSONOpts{ Context: presentation, Operation: imageGenerationOperation, OutputKind: OutputResponse, Format: "auto", Stdout: writer, Stderr: io.Discard, }) @@ -216,12 +209,12 @@ func TestSavedImageCancellationWithoutCompletedFiles(t *testing.T) { root := readableErrorTestCommand(t, "--format-error", format) for _, interrupted := range []bool{false, true} { wrapped := failure - wantStatus := 1 // main's fallback; this check does not execute a native process. + wantStatus := 1 if interrupted { - wrapped = &imageLoadingInterrupt{failure} + wrapped = images.InterruptForTest(failure) wantStatus = 130 } - wrapped = withCommandError(root, wrapped) + wrapped = imageWrapError(root, wrapped) require.ErrorIs(t, wrapped, context.Canceled) status := 1 var exit cli.ExitCoder @@ -230,7 +223,7 @@ func TestSavedImageCancellationWithoutCompletedFiles(t *testing.T) { } require.Equal(t, wantStatus, status) var diagnostic bytes.Buffer - require.NoError(t, ShowCommandError(root, wrapped, &diagnostic)) + require.NoError(t, custom.ShowCommandError(root, wrapped, &diagnostic)) assertReadableErrorContainsNoPrivateDetails(t, diagnostic.String()) require.NotContains(t, diagnostic.String(), directory) require.NotContains(t, diagnostic.String(), encoded) @@ -260,10 +253,10 @@ func TestSavedImageFailureWithoutCompletedFilesKeepsRecovery(t *testing.T) { response := gjson.Parse(`{"data":[{"b64_json":"synthetic-private-invalid-base64"}]}`) for _, format := range []string{"text", "json"} { t.Run(format, func(t *testing.T) { - plan := &imageOutputPlan{directory: t.TempDir(), name: "robot", inline: "off"} + plan := images.NewOutputPlanForTest(t.TempDir(), "robot", "off") var receipt bytes.Buffer - presentation := context.WithValue(t.Context(), imagePresentationKey{}, imagePresentation{plan, &receipt}) - failure := ShowJSON(response, ShowJSONOpts{ + presentation := images.WithPresentationForTest(t.Context(), plan, &receipt) + failure := custom.ShowJSON(response, custom.ShowJSONOpts{ Context: presentation, Operation: imageGenerationOperation, OutputKind: OutputResponse, Format: "auto", Stdout: &receipt, Stderr: io.Discard, }) @@ -271,14 +264,14 @@ func TestSavedImageFailureWithoutCompletedFilesKeepsRecovery(t *testing.T) { require.ErrorAs(t, failure, &malformed) require.False(t, errors.Is(failure, context.Canceled)) require.Empty(t, receipt.String()) - entries, err := os.ReadDir(plan.directory) + entries, err := os.ReadDir(plan.DirectoryForTest()) require.NoError(t, err) require.Empty(t, entries) root := readableErrorTestCommand(t, "--format-error", format) var diagnostic bytes.Buffer - require.NoError(t, ShowCommandError(root, withCommandError(root, failure), &diagnostic)) + require.NoError(t, custom.ShowCommandError(root, imageWrapError(root, failure), &diagnostic)) assertReadableErrorContainsNoPrivateDetails(t, diagnostic.String()) - require.NotContains(t, diagnostic.String(), plan.directory) + require.NotContains(t, diagnostic.String(), plan.DirectoryForTest()) want := "The API responded, but no images could be saved. Check the image data and output folder before trying again." if format == "json" { var document map[string]any @@ -296,19 +289,19 @@ func TestImagePreferenceCancellationKeepsPathPrivate(t *testing.T) { for _, key := range []string{"HOME", "APPDATA", "XDG_CONFIG_HOME"} { t.Setenv(key, home) } - path, err := imageInlinePreferencePath() + path, err := images.TestImageInlinePreferencePath() require.NoError(t, err) ctx, cancel := context.WithCancel(t.Context()) cancel() for _, format := range []string{"text", "json"} { t.Run(format, func(t *testing.T) { root := readableErrorTestCommand(t, "--format-error", format) - failure := imageInlinePreferenceCommands()[0].Action(ctx, root) + failure := images.TestImageInlinePreferenceCommands(custom.ImagePreviewOptions().Receipt)[0].Action(ctx, root) require.ErrorIs(t, failure, context.Canceled) require.Contains(t, failure.Error(), "Could not save the inline preference.") require.NotContains(t, failure.Error(), path) var output bytes.Buffer - require.NoError(t, ShowCommandError(root, withCommandError(root, failure), &output)) + require.NoError(t, custom.ShowCommandError(root, imageWrapError(root, failure), &output)) assertReadableErrorContainsNoPrivateDetails(t, output.String()) require.NotContains(t, output.String(), path) if format == "json" { @@ -328,8 +321,8 @@ func TestImagePreferenceCancellationKeepsPathPrivate(t *testing.T) { func TestSavedImageCancellationOutputModes(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) cancel() - plan := &imageOutputPlan{directory: t.TempDir(), name: "robot", inline: "off"} - failure := plan.save(ctx, []byte(`{"data":[]}`), io.Discard) + plan := images.NewOutputPlanForTest(t.TempDir(), "robot", "off") + failure := plan.SaveForTest(ctx, []byte(`{"data":[]}`), io.Discard) require.ErrorIs(t, failure, context.Canceled) want := "Request canceled." for _, test := range []struct { @@ -345,7 +338,7 @@ func TestSavedImageCancellationOutputModes(t *testing.T) { t.Run(test.name, func(t *testing.T) { root := readableErrorTestCommand(t, test.args...) var output bytes.Buffer - require.NoError(t, ShowCommandError(root, failure, &output)) + require.NoError(t, custom.ShowCommandError(root, failure, &output)) switch test.format { case "text": require.Equal(t, want+"\n", output.String()) @@ -363,7 +356,7 @@ func TestSavedImageCancellationOutputModes(t *testing.T) { require.Equal(t, map[string]any{"message": want}, document) } sinkErr := errors.New("synthetic diagnostic sink failure") - require.ErrorIs(t, ShowCommandError(root, failure, errorSink{sinkErr}), sinkErr) + require.ErrorIs(t, custom.ShowCommandError(root, failure, errorSink{sinkErr}), sinkErr) require.ErrorIs(t, failure, context.Canceled) }) } @@ -390,12 +383,12 @@ func TestSavedImageCancellationAfterReceiptKeepsSavedResult(t *testing.T) { t.Run(inline+"/"+test.name, func(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) defer cancel() - plan := &imageOutputPlan{directory: t.TempDir(), name: "synthetic-private-image", inline: inline} - path := filepath.Join(plan.directory, "synthetic-private-image.png") + plan := images.NewOutputPlanForTest(t.TempDir(), "synthetic-private-image", inline) + path := filepath.Join(plan.DirectoryForTest(), "synthetic-private-image.png") var receipt bytes.Buffer receiptCalls := 0 - // This writer is nonterminal, so auto does not request a preview. - writer := loadingTestWriter(func(data []byte) (int, error) { + + writer := images.TestLoadingTestWriter(func(data []byte) (int, error) { receiptCalls++ require.NoError(t, ctx.Err(), "cancel only after saving and printing the completed path") completed, err := os.ReadFile(path) @@ -405,8 +398,8 @@ func TestSavedImageCancellationAfterReceiptKeepsSavedResult(t *testing.T) { cancel() return n, err }) - presentation := context.WithValue(ctx, imagePresentationKey{}, imagePresentation{plan, writer}) - failure := ShowJSON(response, ShowJSONOpts{ + presentation := images.WithPresentationForTest(ctx, plan, writer) + failure := custom.ShowJSON(response, custom.ShowJSONOpts{ Context: presentation, Operation: imageGenerationOperation, OutputKind: OutputResponse, Format: "auto", Stdout: writer, Stderr: io.Discard, }) @@ -416,19 +409,19 @@ func TestSavedImageCancellationAfterReceiptKeepsSavedResult(t *testing.T) { completed, err := os.ReadFile(path) require.NoError(t, err) require.Equal(t, fixture.Bytes(), completed) - entries, err := os.ReadDir(plan.directory) + entries, err := os.ReadDir(plan.DirectoryForTest()) require.NoError(t, err) require.Len(t, entries, 1) require.Equal(t, filepath.Base(path), entries[0].Name()) root := readableErrorTestCommand(t, test.args...) for _, interrupted := range []bool{false, true} { wrapped := failure - wantStatus := 1 // main's fallback; no native process execution is claimed here. + wantStatus := 1 if interrupted { - wrapped = &imageLoadingInterrupt{failure} + wrapped = images.InterruptForTest(failure) wantStatus = 130 } - wrapped = withCommandError(root, wrapped) + wrapped = imageWrapError(root, wrapped) require.ErrorIs(t, wrapped, context.Canceled) status := 1 var exit cli.ExitCoder @@ -437,9 +430,9 @@ func TestSavedImageCancellationAfterReceiptKeepsSavedResult(t *testing.T) { } require.Equal(t, wantStatus, status) var diagnostic bytes.Buffer - require.NoError(t, ShowCommandError(root, wrapped, &diagnostic)) + require.NoError(t, custom.ShowCommandError(root, wrapped, &diagnostic)) assertReadableErrorContainsNoPrivateDetails(t, diagnostic.String()) - require.NotContains(t, diagnostic.String(), plan.directory) + require.NotContains(t, diagnostic.String(), plan.DirectoryForTest()) require.NotContains(t, diagnostic.String(), encoded) message := diagnostic.String() want := "Images were saved, but the command was canceled. Use the saved files; no need to generate again." @@ -469,3 +462,13 @@ func TestSavedImageCancellationAfterReceiptKeepsSavedResult(t *testing.T) { } } } + +type imageSaveCancellationContext struct { + context.Context + check func() +} + +func (c imageSaveCancellationContext) Err() error { + c.check() + return c.Context.Err() +} diff --git a/internal/commands/images/stream_cleanup_test.go b/internal/commands/images/stream_cleanup_test.go new file mode 100644 index 000000000..75e63a01d --- /dev/null +++ b/internal/commands/images/stream_cleanup_test.go @@ -0,0 +1,43 @@ +package images_test + +import ( + "io" + "testing" + + "github.com/openai/openai-cli/internal/commands/images" + "github.com/openai/openai-cli/pkg/custom" + "github.com/tidwall/gjson" +) + +func TestDeterministicStreamCleanupKeepsSavedImageOwner(t *testing.T) { + for _, event := range []string{ + `{"type":"image_generation.completed","b64_json":"iVBORw0KGgo="}`, + `{"type":"image_generation.failed"}`, + } { + stream := &deterministicCleanupIterator{events: []images.TestOutputJSON{{Result: gjson.Parse(event)}}} + plan := images.NewOutputPlanForTest(t.TempDir(), "synthetic", "off") + ctx := images.WithPresentationForTest(t.Context(), plan, io.Discard) + err := custom.ShowJSONIterator(stream, 0, custom.ShowJSONOpts{Context: ctx, Operation: imageGenerationOperation, OutputKind: OutputStreamEvent}) + failed := gjson.Get(event, "type").String() == "image_generation.failed" + if (err != nil) != failed || stream.reads != 1 || stream.closes != 1 { + t.Fatalf("saved-image owner changed: err=%v reads=%d closes=%d", err, stream.reads, stream.closes) + } + } +} + +type deterministicCleanupIterator struct { + events []images.TestOutputJSON + reads, closes int + errorReadsAfterClose int + sourceErr, closeErr error +} + +func (s *deterministicCleanupIterator) Next() bool { s.reads++; return s.reads <= len(s.events) } +func (s *deterministicCleanupIterator) Current() images.TestOutputJSON { return s.events[s.reads-1] } +func (s *deterministicCleanupIterator) Err() error { + if s.closes != 0 { + s.errorReadsAfterClose++ + } + return s.sourceErr +} +func (s *deterministicCleanupIterator) Close() error { s.closes++; return s.closeErr } diff --git a/internal/commands/images/trust_fixture_test.go b/internal/commands/images/trust_fixture_test.go new file mode 100644 index 000000000..6db4cb9bf --- /dev/null +++ b/internal/commands/images/trust_fixture_test.go @@ -0,0 +1,18 @@ +package images_test + +import ( + "crypto/tls" + "crypto/x509" + "net/http" + "net/http/httptest" + "testing" +) + +func configureTestServerTrust(t *testing.T, server *httptest.Server) { + t.Helper() + serverRoots := x509.NewCertPool() + serverRoots.AddCert(server.Certificate()) + originalTransport := http.DefaultTransport + http.DefaultTransport = &http.Transport{TLSClientConfig: &tls.Config{RootCAs: serverRoots}} + t.Cleanup(func() { http.DefaultTransport = originalTransport }) +} diff --git a/pkg/custom/order_image_commands.go b/internal/commands/order_image_commands.go similarity index 80% rename from pkg/custom/order_image_commands.go rename to internal/commands/order_image_commands.go index d4ca21dae..2e71a05fa 100644 --- a/pkg/custom/order_image_commands.go +++ b/internal/commands/order_image_commands.go @@ -1,4 +1,4 @@ -package custom +package commands import ( "cmp" @@ -7,9 +7,9 @@ import ( "github.com/urfave/cli/v3" ) -// Keep image help and completion candidates in the same task-oriented order. +// OrderImages keeps image help and completion candidates in the same task-oriented order. // The command objects retain their generated handlers, flags and aliases. -func orderImageCommands(root *cli.Command) { +func OrderImages(root *cli.Command) { images := root.Command("images") if images == nil { return diff --git a/pkg/custom/order_image_commands_test.go b/internal/commands/order_image_commands_test.go similarity index 96% rename from pkg/custom/order_image_commands_test.go rename to internal/commands/order_image_commands_test.go index 53fbf8a24..7d733eb54 100644 --- a/pkg/custom/order_image_commands_test.go +++ b/internal/commands/order_image_commands_test.go @@ -1,4 +1,4 @@ -package custom +package commands import ( "context" @@ -24,7 +24,7 @@ func TestOrderImageCommandsPreservesCommands(t *testing.T) { images := &cli.Command{Name: "images", Commands: original} root := &cli.Command{Commands: []*cli.Command{other, images}} for range 2 { // Repeated help configuration must be idempotent. - orderImageCommands(root) + OrderImages(root) want := []string{"generate", "edit", "preview", "models", "inline", "create-variation", "future-b", "future-a"} for i, name := range want { command := images.Commands[i] @@ -44,6 +44,6 @@ func TestOrderImageCommandsPreservesCommands(t *testing.T) { func TestOrderImageCommandsWithoutImages(t *testing.T) { root := &cli.Command{Commands: []*cli.Command{{Name: "models"}}} - orderImageCommands(root) + OrderImages(root) require.Equal(t, "models", root.Commands[0].Name) } diff --git a/pkg/custom/image_loading_feedback.go b/internal/output/loading.go similarity index 63% rename from pkg/custom/image_loading_feedback.go rename to internal/output/loading.go index b2879629d..03ea072a1 100644 --- a/pkg/custom/image_loading_feedback.go +++ b/internal/output/loading.go @@ -1,13 +1,9 @@ -package custom +package output import ( "context" - "errors" "fmt" "io" - "os" - "os/signal" - "runtime" "strings" "sync" "sync/atomic" @@ -15,96 +11,20 @@ import ( "charm.land/bubbles/v2/spinner" "github.com/charmbracelet/x/ansi" - "github.com/charmbracelet/x/term" "github.com/openai/openai-cli/internal/readable" - "github.com/urfave/cli/v3" ) -const imageLoadingDelay = 400 * time.Millisecond +const LoadingDelay = 400 * time.Millisecond -type imageLoadingStage int32 +type LoadingStage int32 const ( - imageLoadingWaiting imageLoadingStage = iota - imageLoadingSaving - imageLoadingSaved + LoadingWaiting LoadingStage = iota + LoadingSaving + LoadingSaved ) -// Feedback starts after request preparation, so it never changes stdin reading -// or validation. It owns no request/output writers and cannot replace a failure. -func runWithImageLoading(ctx context.Context, command *cli.Command, plan *imageOutputPlan, next cli.ActionFunc) (err error) { - root := command.Root() - out := imageCommandWriter(command) - if plan == nil || !isTerminal(out) || - root.Bool("debug") || errorOutputFormat(root) != "text" || root.String("transform-error") != "" { - return next(ctx, command) - } - feedback := os.Stderr - if plan.loadingPrompt != "" { - // Keep the visible picker prompt on its UI surface, outside diagnostics. - feedback = out.(*os.File) // isTerminal above requires an *os.File. - } - if !isTerminal(feedback) { - return next(ctx, command) - } - label := "Generating image" - switch command.Name { - case "edit": - label = "Editing image" - case "create-variation": - label = "Creating image variation" - } - parent := ctx - ctx, stopSignals := signal.NotifyContext(ctx, os.Interrupt) - // Restore normal signal handling once cancellation starts. A second Ctrl-C - // can still terminate the process if a terminal write or cleanup is blocked. - stopReset := context.AfterFunc(ctx, stopSignals) - stopLoading := func() {} - advance := func(imageLoadingStage) {} - if outputDiagnosticsAllowed(ctx) { - stopLoading, advance = startLoadingFeedback(ctx, feedback, label, plan.loadingPrompt, loadingAnimationSupported(os.Getenv), imageLoadingSpinner(os.Getenv, runtime.GOOS), func() (int, int) { - width, height, _ := term.GetSize(feedback.Fd()) - return width, height - }) - } - plan.stopLoading = stopLoading - plan.loadingStage = advance - defer func() { - stopLoading() - stopReset() - interrupted := ctx.Err() != nil && parent.Err() == nil - stopSignals() - var exit cli.ExitCoder - if interrupted && (err == nil || errors.Is(err, context.Canceled)) && !errors.As(err, &exit) { - if err == nil { - err = ctx.Err() - } - err = &imageLoadingInterrupt{err} - } - }() - return next(ctx, command) -} - -// Keep the cancellation cause and any saving guidance while using the shell's -// conventional Ctrl-C status. Ordinary API/local errors keep their own status. -type imageLoadingInterrupt struct{ error } - -func (e *imageLoadingInterrupt) Unwrap() error { return e.error } -func (e *imageLoadingInterrupt) ExitCode() int { return 130 } - -func (p *imageOutputPlan) stopLoadingFeedback() { - if p.stopLoading != nil { - p.stopLoading() - } -} - -func (p *imageOutputPlan) setLoadingStage(stage imageLoadingStage) { - if p != nil && p.loadingStage != nil { - p.loadingStage(stage) - } -} - -func loadingAnimationSupported(getenv func(string) string) bool { +func LoadingAnimationSupported(getenv func(string) string) bool { ci := strings.ToLower(getenv("CI")) if ci != "" && ci != "0" && ci != "false" { return false @@ -118,7 +38,7 @@ func loadingAnimationSupported(getenv func(string) string) bool { return false } -func imageLoadingSpinner(getenv func(string) string, goos string) spinner.Spinner { +func LoadingSpinner(getenv func(string) string, goos string) spinner.Spinner { animation := spinner.Line // Respect locale precedence: LC_ALL=C must not inherit UTF-8 from LANG. locale := getenv("LC_ALL") @@ -140,7 +60,7 @@ func imageLoadingSpinner(getenv func(string) string, goos string) spinner.Spinne // frames does not redact terminal captures. The caller stops this worker before // another writer uses the terminal. Feedback writes are best effort; stop joins // the worker without changing cursor visibility or terminal input modes. -func startLoadingFeedback(ctx context.Context, out io.Writer, label, prompt string, animate bool, animation spinner.Spinner, size func() (int, int)) (stopFeedback func(), advance func(imageLoadingStage)) { +func StartLoadingFeedback(ctx context.Context, out io.Writer, label, prompt string, animate bool, animation spinner.Spinner, size func() (int, int)) (stopFeedback func(), advance func(LoadingStage)) { stop, done := make(chan struct{}), make(chan struct{}) refresh := make(chan struct{}, 1) var stage atomic.Int32 @@ -150,7 +70,7 @@ func startLoadingFeedback(ctx context.Context, out io.Writer, label, prompt stri started := time.Now() go func() { defer close(done) - delay := time.NewTimer(imageLoadingDelay) + delay := time.NewTimer(LoadingDelay) defer delay.Stop() select { case <-stop: @@ -182,10 +102,10 @@ func startLoadingFeedback(ctx context.Context, out io.Writer, label, prompt stri defer func() { if drawn { // Cleanup must still run after request cancellation. - canComplete := !interruptedWrite && ctx.Err() == nil && imageLoadingStage(stage.Load()) == imageLoadingSaved + canComplete := !interruptedWrite && ctx.Err() == nil && LoadingStage(stage.Load()) == LoadingSaved if clear() && canComplete { columns, _ := size() - if line := loadingStatusLine(imageLoadingSaved, time.Since(started), columns); line != "" { + if line := loadingStatusLine(LoadingSaved, time.Since(started), columns); line != "" { // Keep completion visible without retaining the prompt or // delaying saving. Later preview failures remain distinct. _, _ = fmt.Fprintln(out, line) @@ -205,11 +125,11 @@ func startLoadingFeedback(ctx context.Context, out io.Writer, label, prompt stri default: } columns, rows := size() - current := imageLoadingStage(stage.Load()) + current := LoadingStage(stage.Load()) currentLabel := label - if current == imageLoadingSaving { + if current == LoadingSaving { currentLabel = "Saving image" - } else if current == imageLoadingSaved { + } else if current == LoadingSaved { currentLabel = "Images saved" } prefix := animation.Frames[frame%len(animation.Frames)] + " " @@ -261,8 +181,8 @@ func startLoadingFeedback(ctx context.Context, out io.Writer, label, prompt stri return func() { once.Do(func() { close(stop) }) <-done - }, func(next imageLoadingStage) { - if next < imageLoadingWaiting || next > imageLoadingSaved { + }, func(next LoadingStage) { + if next < LoadingWaiting || next > LoadingSaved { return } for { @@ -304,7 +224,7 @@ func loadingPromptLabel(label, prompt string, columns int) string { // Elapsed time describes the request duration. Saving states come from the // existing save workflow; no estimated fraction or extra API work is needed. -func loadingStatusLine(stage imageLoadingStage, elapsed time.Duration, columns int) string { +func loadingStatusLine(stage LoadingStage, elapsed time.Duration, columns int) string { elapsed = max(elapsed, 0) seconds := int64(elapsed / time.Second) duration := fmt.Sprintf("%ds", seconds) @@ -312,7 +232,7 @@ func loadingStatusLine(stage imageLoadingStage, elapsed time.Duration, columns i duration = fmt.Sprintf("%dm %02ds", seconds/60, seconds%60) } labels := []string{duration + " elapsed | Ctrl+C to cancel", duration + " elapsed", duration} - if stage == imageLoadingSaved { + if stage == LoadingSaved { labels = []string{"Images saved | " + duration + " elapsed", "Images saved " + duration, "Images saved"} } for _, label := range labels { diff --git a/pkg/custom/image_loading_feedback_test.go b/internal/output/loading_test.go similarity index 72% rename from pkg/custom/image_loading_feedback_test.go rename to internal/output/loading_test.go index d6f031b3a..53c9bb89c 100644 --- a/pkg/custom/image_loading_feedback_test.go +++ b/internal/output/loading_test.go @@ -1,9 +1,8 @@ -package custom +package output import ( "bytes" "context" - "errors" "io" "strings" "sync" @@ -16,8 +15,6 @@ import ( "charm.land/bubbles/v2/spinner" "github.com/charmbracelet/x/ansi" "github.com/stretchr/testify/require" - "github.com/tidwall/gjson" - "github.com/urfave/cli/v3" ) func TestImageLoadingPromptIsQuotedEscapedAndBounded(t *testing.T) { @@ -46,59 +43,15 @@ func TestImageLoadingPromptIsQuotedEscapedAndBounded(t *testing.T) { require.Equal(t, "Generating image", loadingPromptLabel("Generating image", safe, 20)) } -func TestImageLoadingPromptUsesOnlyCurrentPickerDisplay(t *testing.T) { - const privatePrompt = "SYNTHETIC_INDIRECT_REQUEST_CONTENT" - const visiblePrompt = "Synthetic prompt shown in the picker" - app := &cli.Command{Name: "generate"} - registerImageSavingFlags(app) - app.Action = func(ctx context.Context, command *cli.Command) error { - body := gjson.Parse(`{"prompt":"` + privatePrompt + `","model":"gpt-image-2"}`) - pickerCtx := context.WithValue(ctx, imagePickerLoadingPromptKey{}, visiblePrompt) - for _, test := range []struct { - name string - ctx context.Context - want string - }{ - {"direct request", ctx, ""}, - {"picker request", pickerCtx, visiblePrompt}, - {"reused parent", ctx, ""}, - } { - t.Run(test.name, func(t *testing.T) { - plan, _, err := prepareImageSaving(test.ctx, command, body) - require.NoError(t, err) - require.NotNil(t, plan) - require.Equal(t, privatePrompt, body.Get("prompt").String(), "display selection must not modify request data") - synctest.Test(t, func(t *testing.T) { - var output bytes.Buffer - stop, _ := startLoadingFeedback(t.Context(), &output, "Generating image", plan.loadingPrompt, true, spinner.Line, func() (int, int) { return 100, 24 }) - time.Sleep(imageLoadingDelay + time.Second) - stop() - require.NotContains(t, output.String(), privatePrompt) - require.Contains(t, output.String(), "Generating image") - require.Contains(t, output.String(), "1s elapsed") - if test.want == "" { - require.NotContains(t, output.String(), visiblePrompt) - require.NotContains(t, output.String(), "Generating image '") - } else { - require.Contains(t, output.String(), "Generating image '"+test.want+"'") - } - }) - }) - } - return nil - } - require.NoError(t, app.Run(t.Context(), []string{"openai", "--output-dir", t.TempDir(), "--name", "synthetic-safe-name"})) -} - func TestImageLoadingStatusKeepsElapsedAndSavingWithoutEstimates(t *testing.T) { - for stage := imageLoadingWaiting; stage <= imageLoadingSaved; stage++ { + for stage := LoadingWaiting; stage <= LoadingSaved; stage++ { for _, columns := range []int{10, 20, 35, 80, 100} { line := loadingStatusLine(stage, 65*time.Second, columns) require.Less(t, ansi.StringWidth(line), columns) for _, removed := range []string{"Estimated", "%", "[", "]", "━", "#"} { require.NotContains(t, line, removed) } - if stage == imageLoadingSaved && columns >= 20 { + if stage == LoadingSaved && columns >= 20 { require.Contains(t, line, "Images saved") require.NotContains(t, line, "Ctrl+C") } @@ -107,36 +60,19 @@ func TestImageLoadingStatusKeepsElapsedAndSavingWithoutEstimates(t *testing.T) { } } } - require.Contains(t, loadingStatusLine(imageLoadingWaiting, -time.Second, 80), "0s elapsed") + require.Contains(t, loadingStatusLine(LoadingWaiting, -time.Second, 80), "0s elapsed") for _, elapsed := range []time.Duration{9 * time.Second, 10 * time.Second, 59 * time.Second, time.Minute, time.Hour, time.Duration(1<<63 - 1)} { for columns := 20; columns <= 100; columns++ { - require.Less(t, ansi.StringWidth(loadingStatusLine(imageLoadingWaiting, elapsed, columns)), columns) + require.Less(t, ansi.StringWidth(loadingStatusLine(LoadingWaiting, elapsed, columns)), columns) } } } -type lockedLoadingBuffer struct { - mu sync.Mutex - buffer bytes.Buffer -} - -func (b *lockedLoadingBuffer) Write(p []byte) (int, error) { - b.mu.Lock() - defer b.mu.Unlock() - return b.buffer.Write(p) -} - -func (b *lockedLoadingBuffer) String() string { - b.mu.Lock() - defer b.mu.Unlock() - return b.buffer.String() -} - func TestLoadingFeedbackElapsedAndConfirmedSavingStatus(t *testing.T) { synctest.Test(t, func(t *testing.T) { var output lockedLoadingBuffer - stop, advance := startLoadingFeedback(t.Context(), &output, "Generating image", "synthetic prompt", true, spinner.Line, func() (int, int) { return 100, 24 }) - time.Sleep(imageLoadingDelay) + stop, advance := StartLoadingFeedback(t.Context(), &output, "Generating image", "synthetic prompt", true, spinner.Line, func() (int, int) { return 100, 24 }) + time.Sleep(LoadingDelay) synctest.Wait() require.Contains(t, output.String(), "0s elapsed | Ctrl+C to cancel") time.Sleep(65 * time.Second) @@ -145,14 +81,14 @@ func TestLoadingFeedbackElapsedAndConfirmedSavingStatus(t *testing.T) { require.NotContains(t, output.String(), "Estimated") require.NotContains(t, output.String(), "#") require.NotContains(t, output.String(), "Images saved") - advance(imageLoadingSaving) + advance(LoadingSaving) synctest.Wait() require.Contains(t, output.String(), "1m 05s elapsed") require.Contains(t, output.String(), "Saving image 'synthetic prompt'") - advance(imageLoadingWaiting) + advance(LoadingWaiting) synctest.Wait() require.Contains(t, output.String()[strings.LastIndex(output.String(), "\r\x1b[J"):], "Saving image") - advance(imageLoadingSaved) + advance(LoadingSaved) stop() require.True(t, strings.HasSuffix(output.String(), "Images saved | 1m 05s elapsed\n")) require.NotContains(t, output.String()[strings.LastIndex(output.String(), "\r\x1b[J"):], "synthetic prompt") @@ -165,8 +101,8 @@ func TestLoadingFeedbackStoppedOrCanceledCannotCompleteLater(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) defer cancel() var output bytes.Buffer - stop, advance := startLoadingFeedback(ctx, &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 100, 24 }) - time.Sleep(imageLoadingDelay) + stop, advance := StartLoadingFeedback(ctx, &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 100, 24 }) + time.Sleep(LoadingDelay) synctest.Wait() if cancelFirst { cancel() @@ -175,7 +111,7 @@ func TestLoadingFeedbackStoppedOrCanceledCannotCompleteLater(t *testing.T) { before := output.String() var group sync.WaitGroup for range 8 { - group.Go(func() { advance(imageLoadingSaving); advance(imageLoadingSaved); stop() }) + group.Go(func() { advance(LoadingSaving); advance(LoadingSaved); stop() }) } group.Wait() synctest.Wait() @@ -188,12 +124,12 @@ func TestLoadingFeedbackStoppedOrCanceledCannotCompleteLater(t *testing.T) { func TestLoadingFeedbackDelayAndStop(t *testing.T) { synctest.Test(t, func(t *testing.T) { var output bytes.Buffer - stop, advance := startLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 80, 24 }) - time.Sleep(imageLoadingDelay - time.Nanosecond) + stop, advance := StartLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 80, 24 }) + time.Sleep(LoadingDelay - time.Nanosecond) synctest.Wait() require.Empty(t, output.String()) - advance(imageLoadingSaving) - advance(imageLoadingSaved) + advance(LoadingSaving) + advance(LoadingSaved) stop() stop() time.Sleep(time.Second) @@ -205,15 +141,15 @@ func TestLoadingFeedbackDelayAndStop(t *testing.T) { func TestLoadingFeedbackConcurrentStagesNeverRegress(t *testing.T) { synctest.Test(t, func(t *testing.T) { var output bytes.Buffer - stop, advance := startLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 100, 24 }) - time.Sleep(imageLoadingDelay) + stop, advance := StartLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 100, 24 }) + time.Sleep(LoadingDelay) synctest.Wait() var group sync.WaitGroup for range 8 { group.Go(func() { - advance(imageLoadingSaving) - advance(imageLoadingSaved) - advance(imageLoadingWaiting) + advance(LoadingSaving) + advance(LoadingSaved) + advance(LoadingWaiting) }) } group.Wait() @@ -232,8 +168,8 @@ func TestLoadingFeedbackStopsAndClearsBeforeReturning(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) defer cancel() var output bytes.Buffer - stop, _ := startLoadingFeedback(ctx, &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 80, 24 }) - time.Sleep(imageLoadingDelay + 250*time.Millisecond) + stop, _ := StartLoadingFeedback(ctx, &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 80, 24 }) + time.Sleep(LoadingDelay + 250*time.Millisecond) synctest.Wait() require.Contains(t, output.String(), "\x1b[2K| Generating image") require.Contains(t, output.String(), "\x1b[2K/ Generating image") @@ -261,8 +197,8 @@ func TestLoadingFeedbackStopsAndClearsBeforeReturning(t *testing.T) { func TestLoadingFeedbackShowsElapsedTimeWithoutEstimates(t *testing.T) { synctest.Test(t, func(t *testing.T) { var output bytes.Buffer - stop, _ := startLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 80, 24 }) - time.Sleep(imageLoadingDelay + 65*time.Second) + stop, _ := StartLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 80, 24 }) + time.Sleep(LoadingDelay + 65*time.Second) synctest.Wait() stop() require.Contains(t, output.String(), "0s elapsed | Ctrl+C to cancel") @@ -276,8 +212,8 @@ func TestLoadingFeedbackShowsElapsedTimeWithoutEstimates(t *testing.T) { func TestLoadingFeedbackKeepsElapsedTimeOnNarrowTerminals(t *testing.T) { synctest.Test(t, func(t *testing.T) { var output bytes.Buffer - stop, _ := startLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 20, 24 }) - time.Sleep(imageLoadingDelay + time.Second) + stop, _ := StartLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return 20, 24 }) + time.Sleep(LoadingDelay + time.Second) synctest.Wait() stop() require.Contains(t, output.String(), "1s elapsed") @@ -291,8 +227,8 @@ func TestLoadingFeedbackResizeClearsShorterStatus(t *testing.T) { var output bytes.Buffer var width atomic.Int64 width.Store(80) - stop, _ := startLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return int(width.Load()), 24 }) - time.Sleep(imageLoadingDelay) + stop, _ := StartLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return int(width.Load()), 24 }) + time.Sleep(LoadingDelay) synctest.Wait() width.Store(20) time.Sleep(spinner.Line.FPS) @@ -327,8 +263,8 @@ func TestLoadingFeedbackStaticAndNarrowTerminals(t *testing.T) { t.Run(test.name, func(t *testing.T) { synctest.Test(t, func(t *testing.T) { var output bytes.Buffer - stop, _ := startLoadingFeedback(t.Context(), &output, "Generating image", "", test.animate, spinner.Line, func() (int, int) { return test.width, 24 }) - time.Sleep(imageLoadingDelay + time.Second) + stop, _ := StartLoadingFeedback(t.Context(), &output, "Generating image", "", test.animate, spinner.Line, func() (int, int) { return test.width, 24 }) + time.Sleep(LoadingDelay + time.Second) synctest.Wait() stop() require.Equal(t, test.want, output.String()) @@ -342,8 +278,8 @@ func TestLoadingFeedbackResizeStopsAnimation(t *testing.T) { var output bytes.Buffer var width atomic.Int64 width.Store(80) - stop, _ := startLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return int(width.Load()), 24 }) - time.Sleep(imageLoadingDelay) + stop, _ := StartLoadingFeedback(t.Context(), &output, "Generating image", "", true, spinner.Line, func() (int, int) { return int(width.Load()), 24 }) + time.Sleep(LoadingDelay) synctest.Wait() width.Store(12) time.Sleep(time.Second) @@ -369,8 +305,8 @@ func TestLoadingFeedbackWriteFailureStopsRetries(t *testing.T) { } return 0, io.ErrClosedPipe }) - stop, _ := startLoadingFeedback(t.Context(), writer, "Generating image", "", true, spinner.Line, func() (int, int) { return 80, 24 }) - time.Sleep(imageLoadingDelay + time.Second) + stop, _ := StartLoadingFeedback(t.Context(), writer, "Generating image", "", true, spinner.Line, func() (int, int) { return 80, 24 }) + time.Sleep(LoadingDelay + time.Second) synctest.Wait() stop() wantCalls := 1 @@ -399,11 +335,11 @@ func TestLoadingFeedbackPartialFrameClearsBothRows(t *testing.T) { } return output.Write(p) }) - animation := imageLoadingSpinner(func(key string) string { + animation := LoadingSpinner(func(key string) string { return map[string]string{"TERM": "xterm-256color", "LANG": "en_US.UTF-8"}[key] }, "darwin") - stop, _ := startLoadingFeedback(t.Context(), writer, "Generating image", "synthetic", true, animation, func() (int, int) { return 80, 24 }) - time.Sleep(imageLoadingDelay + time.Second) + stop, _ := StartLoadingFeedback(t.Context(), writer, "Generating image", "synthetic", true, animation, func() (int, int) { return 80, 24 }) + time.Sleep(LoadingDelay + time.Second) synctest.Wait() stop() require.Equal(t, 3, calls, "stop drawing after failure, then attempt one cleanup") @@ -437,7 +373,7 @@ func TestLoadingAnimationEnvironment(t *testing.T) { } return "" } - require.Equal(t, test.want, loadingAnimationSupported(getenv), "TERM=%q CI=%q", test.term, test.ci) + require.Equal(t, test.want, LoadingAnimationSupported(getenv), "TERM=%q CI=%q", test.term, test.ci) } } @@ -472,7 +408,7 @@ func TestImageLoadingSpinnerEnvironment(t *testing.T) { } return "" } - animation := imageLoadingSpinner(getenv, test.goos) + animation := LoadingSpinner(getenv, test.goos) want := spinner.Line if test.unicode { want = spinner.Dot @@ -494,11 +430,11 @@ func TestLoadingFeedbackUnicodeFitsByCells(t *testing.T) { for _, columns := range []int{19, 20} { synctest.Test(t, func(t *testing.T) { var output bytes.Buffer - animation := imageLoadingSpinner(func(key string) string { + animation := LoadingSpinner(func(key string) string { return map[string]string{"TERM": "xterm-256color", "LANG": "en_US.UTF-8"}[key] }, "darwin") - stop, _ := startLoadingFeedback(t.Context(), &output, "Generating image", "", true, animation, func() (int, int) { return columns, 24 }) - time.Sleep(imageLoadingDelay + 2*animation.FPS) + stop, _ := StartLoadingFeedback(t.Context(), &output, "Generating image", "", true, animation, func() (int, int) { return columns, 24 }) + time.Sleep(LoadingDelay + 2*animation.FPS) synctest.Wait() stop() if columns == 19 { @@ -513,64 +449,6 @@ func TestLoadingFeedbackUnicodeFitsByCells(t *testing.T) { } } -func TestImageLoadingLeavesNoninteractiveActionUnchanged(t *testing.T) { - for _, plan := range []*imageOutputPlan{nil, {}} { - command := &cli.Command{Name: "generate", Writer: io.Discard} - ctx := t.Context() - sentinel := errors.New("original action failure") - called := false - err := runWithImageLoading(ctx, command, plan, func(got context.Context, gotCommand *cli.Command) error { - called = true - require.Same(t, ctx, got) - require.Same(t, command, gotCommand) - return sentinel - }) - require.True(t, called) - require.Same(t, sentinel, err) - if plan != nil { - require.Nil(t, plan.stopLoading) - } - } -} - -func TestImageLoadingInterruptRetainsCause(t *testing.T) { - cause := imageSavingFailure("Check saved files before trying again.", context.Canceled) - err := &imageLoadingInterrupt{cause} - require.ErrorIs(t, err, context.Canceled) - var exit cli.ExitCoder - require.ErrorAs(t, err, &exit) - require.Equal(t, 130, exit.ExitCode()) - var saving *imageSavingError - require.ErrorAs(t, err, &saving) - require.Equal(t, cause.Error(), err.Error()) -} - -func TestImageLoadingStopsBeforeVisibleOutput(t *testing.T) { - for _, progress := range []bool{false, true} { - stopped, wrote := false, false - plan := &imageOutputPlan{ - directory: t.TempDir(), name: "synthetic", inline: "off", - stopLoading: func() { stopped = true }, - } - writer := loadingTestWriter(func(p []byte) (int, error) { - require.True(t, stopped, "feedback must stop before any image label, warning or saved path") - wrote = true - return len(p), nil - }) - if progress { - _, err := plan.displayImageProgress(t.Context(), gjson.Parse(`{}`), writer, "kitty") - require.NoError(t, err) - } else { - require.NoError(t, plan.save(t.Context(), []byte(`{"data":[{"b64_json":"iVBORw0KGgo="}]}`), writer)) - } - require.True(t, wrote) - } -} - -type loadingTestWriter func([]byte) (int, error) - -func (w loadingTestWriter) Write(p []byte) (int, error) { return w(p) } - func TestLoadingFeedbackFailedClearDoesNotPrintFallback(t *testing.T) { synctest.Test(t, func(t *testing.T) { var output bytes.Buffer @@ -584,8 +462,8 @@ func TestLoadingFeedbackFailedClearDoesNotPrintFallback(t *testing.T) { } return output.Write(p) }) - stop, _ := startLoadingFeedback(t.Context(), writer, "Generating image", "", true, spinner.Line, func() (int, int) { return int(width.Load()), 24 }) - time.Sleep(imageLoadingDelay) + stop, _ := StartLoadingFeedback(t.Context(), writer, "Generating image", "", true, spinner.Line, func() (int, int) { return int(width.Load()), 24 }) + time.Sleep(LoadingDelay) synctest.Wait() width.Store(12) time.Sleep(time.Second) @@ -601,11 +479,32 @@ func TestLoadingFeedbackShortTerminalUsesStaticPrivateFallback(t *testing.T) { for _, height := range []int{0, 1} { synctest.Test(t, func(t *testing.T) { var output bytes.Buffer - stop, _ := startLoadingFeedback(t.Context(), &output, "Generating image", "synthetic private prompt", true, spinner.Line, func() (int, int) { return 80, height }) - time.Sleep(imageLoadingDelay + time.Second) + stop, _ := StartLoadingFeedback(t.Context(), &output, "Generating image", "synthetic private prompt", true, spinner.Line, func() (int, int) { return 80, height }) + time.Sleep(LoadingDelay + time.Second) synctest.Wait() stop() require.Equal(t, "Generating image\n", output.String()) }) } } + +type lockedLoadingBuffer struct { + mu sync.Mutex + buffer bytes.Buffer +} + +func (b *lockedLoadingBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buffer.Write(p) +} + +func (b *lockedLoadingBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buffer.String() +} + +type loadingTestWriter func([]byte) (int, error) + +func (w loadingTestWriter) Write(p []byte) (int, error) { return w(p) } diff --git a/internal/terminal/writer_terminal.go b/internal/terminal/writer_terminal.go new file mode 100644 index 000000000..71c47d77c --- /dev/null +++ b/internal/terminal/writer_terminal.go @@ -0,0 +1,20 @@ +package terminal + +import ( + "io" + "os" + + "github.com/charmbracelet/x/term" +) + +// IsWriterTerminal recognizes the file and borrowed-file writers used by CLI output. +func IsWriterTerminal(w io.Writer) bool { + switch v := w.(type) { + case BorrowedWriter: + return term.IsTerminal(v.File.Fd()) + case *os.File: + return term.IsTerminal(v.Fd()) + default: + return false + } +} diff --git a/pkg/custom/audio_first_run_review_test.go b/pkg/custom/audio_first_run_review_test.go index cf774971a..16ce0cbd9 100644 --- a/pkg/custom/audio_first_run_review_test.go +++ b/pkg/custom/audio_first_run_review_test.go @@ -7,6 +7,7 @@ import ( "github.com/charmbracelet/x/term" audiocommands "github.com/openai/openai-cli/internal/commands/audio" + "github.com/openai/openai-cli/internal/commands/images" "github.com/stretchr/testify/require" "github.com/urfave/cli/v3" ) @@ -30,7 +31,7 @@ func TestAudioReviewLocalCommandsSuppressFirstRunSetup(t *testing.T) { {"openai", "audio", "--format", "json", leaf}, {"openai", "help", "audio", leaf}, } { - require.True(t, imagePickerFirstRunEligible(args, getenv, true, true, true, shell), "nil-root policy remains unchanged") + require.True(t, images.FirstRunEligible(args, getenv, true, true, true, shell), "nil-root policy remains unchanged") require.True(t, audiocommands.SkipFirstRunSetup(root, args), "registered local audio path must skip setup: %q", args) } } diff --git a/pkg/custom/cmdutil.go b/pkg/custom/cmdutil.go index 496c79f37..7de6d2acb 100644 --- a/pkg/custom/cmdutil.go +++ b/pkg/custom/cmdutil.go @@ -19,6 +19,7 @@ import ( "github.com/charmbracelet/x/term" "github.com/openai/openai-cli/internal/commands/audio" filecommands "github.com/openai/openai-cli/internal/commands/files" + "github.com/openai/openai-cli/internal/commands/images" "github.com/openai/openai-cli/internal/jsonview" clioutput "github.com/openai/openai-cli/internal/output" transformers "github.com/openai/openai-cli/internal/output/transform" @@ -43,16 +44,7 @@ func GetDefaultRequestOptions(cmd *cli.Command) []option.RequestOption { return requestconfig.DefaultOptions(cmd, Version, audio.CaptureText, captureSaveReceiptPolicy(cmd)) } -func isTerminal(w io.Writer) bool { - switch v := w.(type) { - case terminal.BorrowedWriter: - return term.IsTerminal(v.File.Fd()) - case *os.File: - return term.IsTerminal(v.Fd()) - default: - return false - } -} +func isTerminal(w io.Writer) bool { return terminal.IsWriterTerminal(w) } // WriteBinaryResponse writes a binary response to stdout or a file. // @@ -316,10 +308,10 @@ func showJSON(res gjson.Result, opts ShowJSONOpts, selectTransformer transformer if handled, err := filecommands.ShowResult(res, filecommands.ResultOptions{ReadableOptions: fileResultOptions(opts), Quiet: quietOutput(opts.Context)}); handled { return err } - if presentation, ok := savedImagePresentation(opts, OutputResponse); ok { - return presentation.output(func(out io.Writer) error { - return presentation.plan.save(opts.Context, []byte(res.Raw), out) - }) + if isSavedImageRoute(opts, OutputResponse) { + if handled, err := images.ShowSavedResponse(opts.Context, []byte(res.Raw)); handled { + return err + } } if text, ok := audio.TextResult(opts.Context, opts.Operation, opts.OutputKind); ok { if opts.Transform == "" && (strings.EqualFold(opts.Format, "raw") || opts.RawOutput) { @@ -404,10 +396,10 @@ func ShowJSONIterator[T any](iter jsonview.Iterator[T], itemsToDisplay int64, op func showJSONIterator[T any](source jsonview.Iterator[T], itemsToDisplay int64, opts ShowJSONOpts, selectTransformer transformerSelector) (resultErr error) { opts.setDefaults() stopModelsListLoading(opts) - if presentation, ok := savedImagePresentation(opts, OutputStreamEvent); ok { - return presentation.output(func(out io.Writer) error { - return saveFinalImageStream(opts.Context, source, presentation.plan, out) - }) + if isSavedImageRoute(opts, OutputStreamEvent) { + if handled, err := images.ShowSavedStream(opts.Context, source, quietOutput(opts.Context)); handled { + return err + } } // SDK streams own response bodies and timers. Close even when presentation // stops at a limit, cancellation, or an output failure. Saved-image streams diff --git a/pkg/custom/command.go b/pkg/custom/command.go index 1f47f4839..45bff3d02 100644 --- a/pkg/custom/command.go +++ b/pkg/custom/command.go @@ -10,6 +10,7 @@ import ( "github.com/openai/openai-cli/internal/commands/audio" "github.com/openai/openai-cli/internal/commands/codex" filecommands "github.com/openai/openai-cli/internal/commands/files" + "github.com/openai/openai-cli/internal/commands/images" "github.com/openai/openai-cli/internal/commands/manpages" "github.com/openai/openai-cli/internal/commands/tokenize" requestconfig "github.com/openai/openai-cli/internal/request" @@ -33,14 +34,14 @@ func ConfigureCommand(root *cli.Command) { // Batch workflows preserve positional path inputs and need the same help annotations. configureBatchCommands(root) clihelp.DescribeRequestInputs(root) - registerImageModels(root) + images.RegisterModels(root, ImageModelOptions()) configureReadableOutput(root) configureModelsList(root) - configureImageSaving(root) - configureImagePickerCompletion(root) - configureImagePickerShellSetup(root) + images.ConfigureSaving(root, ImageSavingOptions()) + images.ConfigureCompletion(root) + images.ConfigureShellSetup(root, writeOutputReceipt) configureShellSuggestions(root) - registerImagePreviewCommands(root) + images.RegisterPreview(root, ImagePreviewOptions()) audio.ConfigureText(root) configureReadableSpeech(root) registerAdminSetup(root) diff --git a/pkg/custom/deterministic_stream_cleanup_test.go b/pkg/custom/deterministic_stream_cleanup_test.go index 3e35da097..8d774c912 100644 --- a/pkg/custom/deterministic_stream_cleanup_test.go +++ b/pkg/custom/deterministic_stream_cleanup_test.go @@ -209,22 +209,6 @@ func TestDeterministicStreamCleanupRepeatedLoopbackExit(t *testing.T) { } } -func TestDeterministicStreamCleanupKeepsSavedImageOwner(t *testing.T) { - for _, event := range []string{ - `{"type":"image_generation.completed","b64_json":"iVBORw0KGgo="}`, - `{"type":"image_generation.failed"}`, - } { - stream := &deterministicCleanupIterator{events: []outputJSON{{gjson.Parse(event)}}} - plan := &imageOutputPlan{directory: t.TempDir(), name: "synthetic", inline: "off"} - ctx := context.WithValue(t.Context(), imagePresentationKey{}, imagePresentation{plan: plan, writer: io.Discard}) - err := ShowJSONIterator(stream, 0, ShowJSONOpts{Context: ctx, Operation: imageGenerationOperation, OutputKind: OutputStreamEvent}) - failed := gjson.Get(event, "type").String() == "image_generation.failed" - if (err != nil) != failed || stream.reads != 1 || stream.closes != 1 { - t.Fatalf("saved-image owner changed: err=%v reads=%d closes=%d", err, stream.reads, stream.closes) - } - } -} - type deterministicSpeechBody struct { io.Reader closes int diff --git a/pkg/custom/flagoptions.go b/pkg/custom/flagoptions.go index a7c8d1c7d..b6521b4f0 100644 --- a/pkg/custom/flagoptions.go +++ b/pkg/custom/flagoptions.go @@ -7,6 +7,7 @@ import ( "github.com/openai/openai-cli/internal/apiquery" "github.com/openai/openai-cli/internal/commands/admin" "github.com/openai/openai-cli/internal/commands/audio" + "github.com/openai/openai-cli/internal/commands/images" requestconfig "github.com/openai/openai-cli/internal/request" "github.com/openai/openai-go/v3/option" "github.com/urfave/cli/v3" @@ -48,22 +49,17 @@ func flagOptionsWithStdin( stdin io.Reader, observeJSON ...func([]byte), ) (options []option.RequestOption, err error) { - if prepared, ok, err := consumeImageSavingRequest(cmd, nestedFormat, arrayFormat, bodyType, ignoreStdin); ok { + if prepared, ok, err := images.ConsumeSavingRequest(cmd, nestedFormat, arrayFormat, bodyType, ignoreStdin); ok { return prepared, err } hooks := requestconfig.PreparationHooks{ PreflightMultipart: func(body map[string]any) error { return audio.PreflightVoiceBody(cmd, body) }, ValidateCredentials: func() error { return admin.CheckCredentials(cmd) }, Encoding: requestconfig.EncodingHooks{ - PrepareMultipart: func(body map[string]any) error { return prepareImageMultipartBody(cmd, body) }, + PrepareMultipart: func(body map[string]any) error { return images.PrepareMultipartBody(cmd, body) }, ValidateJSON: func(body []byte) error { return validateProjectAPIKeyUpdate(cmd, body) }, ObserveJSON: observeJSON, - ObserveMultipart: func(body io.Closer) { - // Look up the feature state only when the encoded body is ready. - if state, ok := cmd.Metadata[imageMultipartMetadata].(*imageMultipartPreparation); ok { - state.body = body - } - }, + ObserveMultipart: func(body io.Closer) { images.ObserveMultipart(cmd, body) }, }, } if audio.IsVoiceAction(cmd) { diff --git a/pkg/custom/help.go b/pkg/custom/help.go index 0adca0323..022ba7f4e 100644 --- a/pkg/custom/help.go +++ b/pkg/custom/help.go @@ -4,7 +4,9 @@ import ( "strings" "github.com/openai/openai-cli/internal/clihelp" + localcommands "github.com/openai/openai-cli/internal/commands" "github.com/openai/openai-cli/internal/commands/admin" + imagecommands "github.com/openai/openai-cli/internal/commands/images" requestconfig "github.com/openai/openai-cli/internal/request" "github.com/openai/openai-cli/internal/requestflag" "github.com/urfave/cli/v3" @@ -12,7 +14,7 @@ import ( // ConfigureHelp decorates help without changing API commands or their defaults. func ConfigureHelp(root *cli.Command, args []string) ([]string, bool, error) { - orderImageCommands(root) + localcommands.OrderImages(root) configureGlobalFlagDescriptions(root) configureHelpGroups(root) configureImageHelpContent(root) @@ -140,7 +142,7 @@ func configureImageHelpContent(root *cli.Command) { Examples: []clihelp.Example{{Description: "Create a variation with images edit:", Command: `images edit --image "photo.png" --prompt "Create a variation of this image" --name variation`}}, }, "preview": { - Description: imagePreviewDetails + "\nReplace photo.png with your saved image's path.\n" + + Description: imagecommands.PreviewDetails + "\nReplace photo.png with your saved image's path.\n" + "Pipes, CI and terminals without supported graphics or color cannot display previews.\n" + "Preview limits: 64 MiB and 16 megapixels. Larger originals are still kept.", Examples: []clihelp.Example{{Description: "View a saved image:", Command: `images preview "photo.png"`}}, diff --git a/pkg/custom/help_test.go b/pkg/custom/help_test.go index c568bfbfd..bfffc8599 100644 --- a/pkg/custom/help_test.go +++ b/pkg/custom/help_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/openai/openai-cli/internal/clihelp" + imagecommands "github.com/openai/openai-cli/internal/commands/images" requestconfig "github.com/openai/openai-cli/internal/request" "github.com/openai/openai-cli/internal/requestflag" "github.com/urfave/cli/v3" @@ -155,9 +156,9 @@ func TestConfigureHelpPreservesImageGuidance(t *testing.T) { {Name: "unrelated", Description: "Keep this description", CustomHelpTemplate: "Keep this template"}, }} root := &cli.Command{Name: "openai", Commands: []*cli.Command{images}} - configureImageSaving(root) - registerImagePreviewCommands(root) - registerImageModels(root) + imagecommands.ConfigureSaving(root, ImageSavingOptions()) + imagecommands.RegisterPreview(root, ImagePreviewOptions()) + imagecommands.RegisterModels(root, ImageModelOptions()) wantGuidance := map[string][]string{ "generate": {"interactive terminal", "picker reopens", "Ctrl+C exits", "~/Downloads/gpt-images/", "--format json", "without --name or --output-dir", "depend on the model"}, "edit": {"Replace photo.png", "Keeps your original", "~/Downloads/gpt-images/", "existing folder", "--format json", "depend on the model"}, diff --git a/pkg/custom/image_first_run_admission_test.go b/pkg/custom/image_first_run_admission_test.go new file mode 100644 index 000000000..c539702b6 --- /dev/null +++ b/pkg/custom/image_first_run_admission_test.go @@ -0,0 +1,63 @@ +package custom + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestImagePickerFirstRunBlockedWorkDoesNotBlockCommand(t *testing.T) { + for _, canceled := range []bool{false, true} { + t.Run(map[bool]string{false: "deadline", true: "cancellation"}[canceled], func(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + started, unblock, foregroundDone, workerDone := make(chan struct{}), make(chan struct{}), make(chan struct{}), make(chan struct{}) + var release sync.Once + t.Cleanup(func() { release.Do(func() { close(unblock) }) }) + workerContext := make(chan context.Context, 1) + go func() { + waitForImagePickerFirstRun(ctx, func(ctx context.Context) { + workerContext <- ctx + close(started) + <-unblock // Models an OS operation that ignores context cancellation. + close(workerDone) + }) + close(foregroundDone) + }() + select { + case <-started: + case <-time.After(3 * time.Second): + t.Fatal("setup worker did not start") + } + if canceled { + cancel() + } + select { + case <-foregroundDone: + case <-time.After(2 * time.Second): + t.Fatal("optional blocked I/O prevented the foreground command from continuing") + } + require.Error(t, (<-workerContext).Err()) + secondStarted := make(chan struct{}, 1) + waitForImagePickerFirstRun(t.Context(), func(context.Context) { secondStarted <- struct{}{} }) + require.Empty(t, secondStarted, "a blocked worker must not accumulate more work") + release.Do(func() { close(unblock) }) + <-workerDone + // The worker releases admission after its callback returns. + require.Eventually(t, func() bool { return len(imagePickerFirstRunWorker) == 0 }, time.Second, time.Millisecond) + waitForImagePickerFirstRun(t.Context(), func(context.Context) { secondStarted <- struct{}{} }) + require.Len(t, secondStarted, 1, "finished work must release the slot") + }) + } +} + +func TestImagePickerFirstRunAlreadyCanceledDoesNotStartWork(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + started := make(chan struct{}, 1) + waitForImagePickerFirstRun(ctx, func(context.Context) { started <- struct{}{} }) + require.Empty(t, started) +} diff --git a/pkg/custom/image_models_test.go b/pkg/custom/image_models_test.go deleted file mode 100644 index 27c1b2509..000000000 --- a/pkg/custom/image_models_test.go +++ /dev/null @@ -1,277 +0,0 @@ -package custom - -import ( - "context" - "crypto/tls" - "crypto/x509" - "encoding/json" - "errors" - "fmt" - "io" - "log" - "net/http" - "net/http/httptest" - "strings" - "sync/atomic" - "testing" - - "github.com/openai/openai-cli/internal/imagemodels" - mtlstest "github.com/openai/openai-cli/internal/testutil/mtls" - "github.com/urfave/cli/v3" -) - -func imageModelTestRow(id string, status imagemodels.Status, failure imagemodels.Failure) imagemodels.Result { - return imagemodels.Result{Entry: imagemodels.Entry{ID: id}, Status: status, Failure: failure} -} - -func TestImageModelsPresentationPartialAndHidden(t *testing.T) { - report := imageModelsReport{Source: "live", Models: []imagemodels.Result{ - imageModelTestRow("gpt-image-2.5-sunburst", imagemodels.StatusVisible, ""), - imageModelTestRow("gpt-image-2.5-flare", imagemodels.StatusUnknown, imagemodels.FailureTimeout), - imageModelTestRow("gpt-image-1", imagemodels.StatusNotVisible, ""), - imageModelTestRow("dall-e-2", imagemodels.StatusRetired, ""), - }} - var out strings.Builder - if err := writeImageModels(&out, report, false, "openai", &out); err != nil { - t.Fatal(err) - } - for _, want := range []string{"gpt-image-2.5-sunburst", "gpt-image-2.5-flare", "Could not check (timeout)", "2 retired or not visible", "openai images models --all", `openai images generate --prompt "A tiny orange robot" --model gpt-image-2.5-sunburst`, "generation permissions can differ"} { - if !strings.Contains(out.String(), want) { - t.Errorf("missing %q: %s", want, out.String()) - } - } - for _, unwanted := range []string{"gpt-image-1", "dall-e-2", "default", "No known", "{", "\x1b"} { - if strings.Contains(out.String(), unwanted) { - t.Errorf("unexpected %q: %s", unwanted, out.String()) - } - } - out.Reset() - if err := writeImageModels(&out, report, true, "openai", &out); err != nil { - t.Fatal(err) - } - if !strings.Contains(out.String(), "gpt-image-1") || !strings.Contains(out.String(), "dall-e-2") { - t.Fatalf("--all lost results: %s", out.String()) - } -} - -func TestImageModelsSuggestsOnlyVisibleOrUncheckedNames(t *testing.T) { - for _, status := range []imagemodels.Status{imagemodels.StatusVisible, imagemodels.StatusNotChecked, imagemodels.StatusRetired, imagemodels.StatusNotVisible, imagemodels.StatusUnknown} { - t.Run(string(status), func(t *testing.T) { - report := imageModelsReport{Source: "live", Models: []imagemodels.Result{imageModelTestRow("gpt-image-1", status, imagemodels.FailureTimeout)}} - if status == imagemodels.StatusNotChecked { - report.Source = "offline" - } - for _, all := range []bool{false, true} { - var out strings.Builder - if err := writeImageModels(&out, report, all, "openai", &out); err != nil { - t.Fatal(err) - } - eligible := status == imagemodels.StatusVisible || status == imagemodels.StatusNotChecked - if strings.Contains(out.String(), "Choose a model") != eligible { - t.Errorf("misleading suggestion: %s", out.String()) - } - if strings.Contains(out.String(), "default") { - t.Errorf("claimed an unshipped default: %s", out.String()) - } - if status == imagemodels.StatusUnknown && (strings.Contains(out.String(), "No known") || !strings.Contains(out.String(), "Could not check (timeout)")) { - t.Errorf("failed check claimed unavailable: %s", out.String()) - } - if status == imagemodels.StatusNotChecked && (!strings.Contains(out.String(), "access not checked (offline)") || strings.Contains(out.String(), "Visible")) { - t.Errorf("offline claimed visibility: %s", out.String()) - } - } - }) - } -} - -func TestImageModelsStatusAndFailureGuidance(t *testing.T) { - for _, tc := range []struct { - row imagemodels.Result - want string - }{ - {imagemodels.Result{Status: imagemodels.StatusVisible, ShutdownDate: "2099-01-01"}, "Visible (retires 2099-01-01)"}, - {imagemodels.Result{Status: imagemodels.StatusRetired, ShutdownDate: "2000-01-01"}, "Retired 2000-01-01"}, - {imagemodels.Result{Status: imagemodels.StatusUnknown, Failure: imagemodels.FailureAuthentication}, "Could not check (authentication)"}, - {imagemodels.Result{Status: imagemodels.StatusUnknown, Failure: imagemodels.FailureForbidden}, "Could not check (access denied)"}, - {imagemodels.Result{Status: imagemodels.StatusUnknown, Failure: imagemodels.FailureRateLimit}, "Could not check (rate limit)"}, - } { - if got := imageModelStatusText(tc.row); got != tc.want { - t.Errorf("status=%q; want %q", got, tc.want) - } - } - for failure, want := range map[imagemodels.Failure]string{ - imagemodels.FailureAuthentication: "did not accept authentication", imagemodels.FailureForbidden: "denied access", - imagemodels.FailureRateLimit: "rate-limited", imagemodels.FailureTimeout: "timed out", imagemodels.FailureServer: "API could not complete", - imagemodels.FailureNetwork: "Could not reach", imagemodels.FailureInvalidResponse: "unexpected model response", imagemodels.FailureCanceled: "could not be completed", - } { - message := imageModelsFailureMessage([]imagemodels.Result{{Status: imagemodels.StatusUnknown, Failure: failure}}, "openai") - if !strings.Contains(message, want) || !strings.Contains(message, "openai images models --offline") { - t.Errorf("failure %s: %s", failure, message) - } - } -} - -func TestImageModelsErrorClassification(t *testing.T) { - const private = "synthetic-private-detail\x1b]52;c;synthetic-private-control\a" - for _, extraArguments := range []bool{false, true} { - diagnostic := &imageModelsError{ - invocation: "openai", extraArguments: extraArguments, - results: []imagemodels.Result{ - {Entry: imagemodels.Entry{ID: private}, Status: imagemodels.StatusUnknown, Failure: imagemodels.Failure(private), ShutdownDate: private}, - {Status: imagemodels.StatusUnknown, Failure: imagemodels.FailureAuthentication}, - }, - } - wrapped := fmt.Errorf("%s: %w", private, diagnostic) - want := diagnostic.Error() - if got := localErrorMessage(nil, wrapped); got != want || strings.Contains(got, "synthetic-private-") { - t.Errorf("typed diagnostic = %q, want controlled guidance %q", got, want) - } - for _, arbitrary := range []error{errors.New(want), errors.New(want + private), errors.New(private)} { - if got := localErrorMessage(nil, arbitrary); got != "The command could not be completed. Check your arguments with --help." { - t.Errorf("untyped diagnostic became trusted: %q", got) - } - } - for _, format := range []string{"text", "json"} { - root := readableErrorTestCommand(t, "--format-error", format) - var out strings.Builder - if err := ShowCommandError(root, wrapped, &out); err != nil { - t.Fatal(err) - } - message := strings.TrimSpace(out.String()) - if format == "json" { - var payload struct{ Message string } - if err := json.Unmarshal([]byte(message), &payload); err != nil { - t.Fatal(err) - } - message = payload.Message - } - if message != want { - t.Errorf("%s diagnostic = %q, want %q", format, message, want) - } - } - if got := localErrorMessage(nil, errors.Join(wrapped, context.Canceled)); got != "Request canceled." { - t.Errorf("cancellation lost precedence: %q", got) - } - } -} - -type imageModelsFailWriter struct{ err error } - -func (w imageModelsFailWriter) Write([]byte) (int, error) { return 0, w.err } - -func TestImageModelsWriteErrors(t *testing.T) { - want := errors.New("synthetic write failure") - for _, failure := range []error{want, nil} { - expected := failure - if expected == nil { - expected = io.ErrShortWrite - } - if got := writeImageModels(imageModelsFailWriter{failure}, imageModelsReport{}, false, "openai", nil); !errors.Is(got, expected) { - t.Fatalf("error=%v; want %v", got, expected) - } - } -} - -func TestImageModelsUsesConfiguredMTLSClient(t *testing.T) { - t.Setenv("OPENAI_API_KEY", "synthetic-models-key") - pki := mtlstest.NewPKI(t) - clientCAs := x509.NewCertPool() - if !clientCAs.AppendCertsFromPEM(pki.RootPEM) { - t.Fatal("could not configure synthetic client CA") - } - var requests atomic.Int32 - server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - requests.Add(1) - if r.TLS == nil || len(r.TLS.VerifiedChains) == 0 || len(r.TLS.VerifiedChains[0]) != 3 { - t.Error("model check lost the configured client certificate chain") - } - if r.Header.Get("Authorization") != "Bearer synthetic-models-key" || r.Method != http.MethodGet || !strings.HasPrefix(r.URL.Path, "/v1/models/") { - t.Error("model check changed the configured credentials, method or base path") - } - w.Header().Set("Content-Type", "application/json") - fmt.Fprintf(w, `{"id":%q,"object":"model"}`, strings.TrimPrefix(r.URL.Path, "/v1/models/")) - })) - server.Config.ErrorLog = log.New(io.Discard, "", 0) - server.TLS = &tls.Config{ClientAuth: tls.RequireAndVerifyClientCert, ClientCAs: clientCAs, MinVersion: tls.VersionTLS12} - server.StartTLS() - t.Cleanup(server.Close) - configureTestServerTrust(t, server) - certFile, keyFile := mtlstest.WriteClientFiles(t, pki.ClientChainPEM, pki.ClientKeyPEM) - var out, stderr strings.Builder - root := &cli.Command{ - Name: "openai", Writer: &out, ErrWriter: &stderr, - Flags: []cli.Flag{&cli.StringFlag{Name: "base-url"}, &cli.StringFlag{Name: "format", Value: "json"}}, - Commands: []*cli.Command{{Name: "images"}}, - } - ConfigureCommand(root) - if err := root.Run(t.Context(), []string{"openai", "--base-url", server.URL + "/v1/", - "--mtls-client-cert-file", certFile, "--mtls-client-key-file", keyFile, "images", "models"}); err != nil { - t.Fatal(err) - } - var report imageModelsReport - if err := json.Unmarshal([]byte(out.String()), &report); err != nil { - t.Fatal(err) - } - if requests.Load() != 9 || !report.Complete || len(report.Models) != 9 || stderr.Len() != 0 { - t.Fatalf("mTLS discovery did not complete all model checks: requests=%d; report=%+v; stderr=%q", requests.Load(), report, stderr.String()) - } -} - -func TestImageModelsDataFormatsAndExtraction(t *testing.T) { - report := imageModelsReport{Source: "offline", Models: []imagemodels.Result{imageModelTestRow("gpt-image-2.5-sunburst", imagemodels.StatusNotChecked, "")}} - for _, format := range []string{"auto", "text", "JSON", "jsonl", "raw", "yaml", "explore"} { - t.Run(format, func(t *testing.T) { - var out, stderr strings.Builder - command := imageModelsOutputCommand(&out, &stderr, report) - if err := command.Run(context.Background(), []string{"openai", "--format", format}); err != nil { - t.Fatal(err) - } - if !strings.Contains(out.String(), "gpt-image-2.5-sunburst") || !strings.Contains(out.String(), "not_checked") || strings.Contains(out.String(), "default") { - t.Fatalf("incorrect report: %q", out.String()) - } - if format == "auto" || format == "text" { - if json.Valid([]byte(out.String())) || !strings.Contains(out.String(), "Source: offline") { - t.Fatalf("expected text: %q", out.String()) - } - } else if format != "yaml" && !json.Valid([]byte(out.String())) { - t.Fatalf("invalid JSON: %q", out.String()) - } - if format == "explore" && !strings.Contains(stderr.String(), "falling back to 'json'") { - t.Fatalf("missing existing explore warning: %q", stderr.String()) - } - }) - } - for _, format := range []string{"auto", "text", "json", "jsonl", "raw", "yaml"} { - for _, raw := range []bool{false, true} { - var out, stderr strings.Builder - command := imageModelsOutputCommand(&out, &stderr, report) - args := []string{"openai", "--format", format, "--transform", "models.0.id"} - if raw { - args = append(args, "--raw-output") - } - if err := command.Run(context.Background(), args); err != nil { - t.Fatal(err) - } - want := "gpt-image-2.5-sunburst\n" - if !raw && (format == "auto" || format == "json" || format == "jsonl" || format == "raw") { - want = "\"gpt-image-2.5-sunburst\"\n" - } - if out.String() != want { - t.Errorf("format=%s raw=%v got %q want %q", format, raw, out.String(), want) - } - } - } - var stderr strings.Builder - want := errors.New("synthetic data output failure") - if err := imageModelsOutputCommand(imageModelsFailWriter{want}, &stderr, report).Run(context.Background(), []string{"openai", "--format", "json"}); !errors.Is(err, want) { - t.Errorf("lost output failure: %v", err) - } -} - -func imageModelsOutputCommand(out, stderr io.Writer, report imageModelsReport) *cli.Command { - return &cli.Command{Name: "openai", Writer: out, Flags: []cli.Flag{ - &cli.StringFlag{Name: "format", Value: "auto"}, &cli.StringFlag{Name: "transform"}, &cli.BoolFlag{Name: "raw-output"}, - }, Action: func(ctx context.Context, command *cli.Command) error { - return writeImageModelsData(ctx, command, report, stderr) - }} -} diff --git a/pkg/custom/image_picker_first_run.go b/pkg/custom/image_picker_first_run.go index f70bdb129..657967116 100644 --- a/pkg/custom/image_picker_first_run.go +++ b/pkg/custom/image_picker_first_run.go @@ -3,14 +3,11 @@ package custom import ( "context" "os" - "os/exec" - "strings" "time" "github.com/charmbracelet/x/term" - "github.com/openai/openai-cli/internal/autocomplete" audiocommands "github.com/openai/openai-cli/internal/commands/audio" - cliterminal "github.com/openai/openai-cli/internal/terminal" + "github.com/openai/openai-cli/internal/commands/images" "github.com/urfave/cli/v3" ) @@ -35,7 +32,7 @@ func SetupImagePickerShellOnFirstRun(ctx context.Context, args []string, roots . } args = append([]string(nil), args...) waitForImagePickerFirstRun(ctx, func(ctx context.Context) { - setupImagePickerShellOnFirstRun(ctx, args) + images.SetupShellOnFirstRun(ctx, args) }) } @@ -67,133 +64,3 @@ func waitForImagePickerFirstRun(ctx context.Context, setup func(context.Context) case <-done: } } - -func setupImagePickerShellOnFirstRun(ctx context.Context, args []string) { - // The immediate shell may differ from the account's login shell. - shell := cliterminal.ParentShell(ctx) - if !imagePickerFirstRunEligible(args, os.Getenv, true, true, true, shell) { - return - } - executable, err := os.Executable() - if err != nil { - return - } - onPath, err := exec.LookPath("openai") - if err != nil || !imagePickerSameExecutable(executable, onPath) { - return - } - targets, err := imagePickerShellTarget(ctx, shell, true, "") - if err != nil { - return - } - _ = setupImagePickerFirstRun(ctx, targets) -} - -func imagePickerFirstRunEligible(args []string, getenv func(string) string, inputTTY, outputTTY, errorTTY bool, shell string) bool { - switch shell { - case "bash", "zsh", "fish": - default: - return false - } - if len(args) == 0 || !inputTTY || !outputTTY || !errorTTY || strings.EqualFold(getenv("TERM"), "dumb") || - getenv("OPENAI_PICKER_SHELL") != "" { - return false - } - for _, name := range []string{"CI", "GITHUB_ACTIONS", "GITLAB_CI", "TF_BUILD", "BUILDKITE", "JENKINS_URL", "TEAMCITY_VERSION"} { - value := strings.ToLower(getenv(name)) - if value != "" && value != "false" && value != "0" { - return false - } - } - // Conservatively ignore internal operations anywhere in argv. Inspecting - // these fixed tokens never parses, consumes, or rewrites request arguments. - for _, argument := range args[1:] { - switch argument { - case "__complete", "@completion", "@manpages", "--generate-shell-completion", "tokenizer", "codex": - return false - } - } - return true -} - -func imagePickerSameExecutable(executable, onPath string) bool { - current, err := os.Stat(executable) - if err != nil || !current.Mode().IsRegular() { - return false - } - resolved, err := os.Stat(onPath) - return err == nil && resolved.Mode().IsRegular() && os.SameFile(current, resolved) -} - -func setupImagePickerFirstRun(ctx context.Context, targets []autocomplete.PickerInstallation) error { - if err := ctx.Err(); err != nil || len(targets) == 0 { - return err - } - for _, target := range targets { - if target.Shell == autocomplete.CompletionStylePowershell { - return nil - } - } - missing, err := imagePickerFirstRunMissing(ctx, targets) - if err != nil { - return err - } - if len(missing) == 0 { - legacy, err := legacyImagePickerTabChoicePath(string(targets[0].Shell)) - if err != nil { - return err - } - declined, err := readImagePickerTabDecline(legacy) - if err != nil || !declined { - return err - } - } - directory, err := imagePickerTabChoiceDirectory() - if err != nil { - return err - } - return autocomplete.WithPickerSetupLock(ctx, directory, func() error { - if err := migrateImagePickerTabDecline(ctx, string(targets[0].Shell)); err != nil { - return err - } - // Recheck preference and every startup file after taking the same - // decision lock as explicit installation/removal. An opt-out or a - // changed profile while waiting must not be overwritten. - missing, err := imagePickerFirstRunMissing(ctx, targets) - if err != nil { - return err - } - for _, target := range missing { - if _, err := autocomplete.InstallPicker(ctx, target); err != nil { - return err - } - } - return nil - }) -} - -func imagePickerFirstRunMissing(ctx context.Context, targets []autocomplete.PickerInstallation) ([]autocomplete.PickerInstallation, error) { - if err := ctx.Err(); err != nil || len(targets) == 0 { - return nil, err - } - for _, target := range targets { - if target.Shell == autocomplete.CompletionStylePowershell { - return nil, nil - } - } - declined, err := imagePickerTabDeclined(string(targets[0].Shell)) - if err != nil || declined { - return nil, err - } - var missing []autocomplete.PickerInstallation - for _, target := range targets { - installed, err := autocomplete.IsPickerInstalled(ctx, target) - if err != nil { - return nil, err - } - if !installed { - missing = append(missing, target) - } - } - return missing, nil -} diff --git a/pkg/custom/images_policy.go b/pkg/custom/images_policy.go new file mode 100644 index 000000000..8b83dd241 --- /dev/null +++ b/pkg/custom/images_policy.go @@ -0,0 +1,53 @@ +package custom + +import ( + "context" + "io" + + "github.com/openai/openai-cli/internal/commands/images" + "github.com/tidwall/gjson" + "github.com/urfave/cli/v3" +) + +// ImageModelOptions binds discovery to application policy. +func ImageModelOptions() images.ModelOptions { + return images.ModelOptions{ + RequestOptions: FlagOptions, + DefaultRequestOptions: GetDefaultRequestOptions, + DiagnosticsAllowed: outputDiagnosticsAllowed, + HumanOutput: func(command *cli.Command) bool { + root := command.Root() + return root.String("transform") == "" && !root.Bool("raw-output") && resolvedOutputFormat(ShowJSONOpts{Format: root.String("format")}) == "text" + }, + ShowData: func(ctx context.Context, command *cli.Command, obj gjson.Result, stderr io.Writer) error { + root := command.Root() + return ShowJSON(obj, ShowJSONOpts{ + Context: ctx, Format: root.String("format"), ExplicitFormat: root.IsSet("format"), + RawOutput: root.Bool("raw-output"), Title: "Image models", Transform: root.String("transform"), + Stdout: root.Writer, Stderr: stderr, + }) + }, + } +} + +func isSavedImageRoute(opts ShowJSONOpts, kind OutputKind) bool { + if opts.OutputKind != kind || opts.Context == nil { + return false + } + switch opts.Operation { + case "(resource) images > (method) generate", "(resource) images > (method) edit", "(resource) images > (method) create_variation": + return true + default: + return false + } +} + +// ImageSavingOptions binds image preparation to application request and diagnostic policy. +func ImageSavingOptions() images.SavingOptions { + return images.SavingOptions{RequestOptions: FlagOptions, DiagnosticsAllowed: outputDiagnosticsAllowed, ErrorFormat: errorOutputFormat} +} + +// ImagePreviewOptions binds local previews to application receipt and error policy. +func ImagePreviewOptions() images.PreviewOptions { + return images.PreviewOptions{Receipt: writeOutputReceipt, ShowError: ShowCommandError} +} diff --git a/pkg/custom/local_errors.go b/pkg/custom/local_errors.go index 90b999698..42b087d01 100644 --- a/pkg/custom/local_errors.go +++ b/pkg/custom/local_errors.go @@ -17,6 +17,7 @@ import ( "github.com/openai/openai-cli/internal/commands/admin" "github.com/openai/openai-cli/internal/commands/audio" batchcommands "github.com/openai/openai-cli/internal/commands/batches" + "github.com/openai/openai-cli/internal/commands/images" "github.com/openai/openai-cli/internal/commands/models" clioutput "github.com/openai/openai-cli/internal/output" "github.com/openai/openai-cli/internal/output/listview" @@ -42,10 +43,10 @@ func localErrorMessage(root *cli.Command, failure error) string { var inputFailure *requestconfig.InputFileError var stdinFailure *requestflag.StdinValueError var pagerFailure *clioutput.PagerError - var imageFailure *imageSavingError + var imageFailure *images.SavingError var scalarFailure *requestconfig.MultipartScalarError var streamFailure *streamResultError - var imageModelsFailure *imageModelsError + var imageModelsFailure *images.ModelsError var projectAPIKeyUpdateFailure *projectAPIKeyUpdateError var voiceTypeFailure *audio.VoiceTypeError var helpTopicFailure *clihelp.UnknownTopicError @@ -68,8 +69,8 @@ func localErrorMessage(root *cli.Command, failure error) string { case errors.As(failure, &receiptFailure): return receiptFailure.Error() case errors.Is(failure, context.Canceled): - if errors.As(failure, &imageFailure) && imageFailure.preserveOnCancel { - return imageFailure.message + if errors.As(failure, &imageFailure) && imageFailure.PreserveOnCancel() { + return imageFailure.Error() } return "Request canceled." case errors.As(failure, &utilityFailure): @@ -81,7 +82,7 @@ func localErrorMessage(root *cli.Command, failure error) string { case errors.As(failure, &finiteListFailure): return finiteListFailure.Error() case errors.As(failure, &imageFailure): - return imageFailure.message + return imageFailure.Error() case errors.As(failure, &scalarFailure): return scalarFailure.Error() case errors.As(failure, &navigationFailure): diff --git a/pkg/custom/local_utilities_test.go b/pkg/custom/local_utilities_test.go index fa8cf4f5f..a5091e93c 100644 --- a/pkg/custom/local_utilities_test.go +++ b/pkg/custom/local_utilities_test.go @@ -6,6 +6,7 @@ import ( "testing" localcommands "github.com/openai/openai-cli/internal/commands" + "github.com/openai/openai-cli/internal/commands/images" "github.com/urfave/cli/v3" ) @@ -69,7 +70,7 @@ func TestLocalUtilityFirstRunSuppression(t *testing.T) { // Temporary HOME process tests also encounter the later real-home guard. for _, shell := range []string{"bash", "zsh", "fish"} { getenv := func(string) string { return "" } - if !imagePickerFirstRunEligible([]string{"openai", "models", "list"}, getenv, true, true, true, shell) { + if !images.FirstRunEligible([]string{"openai", "models", "list"}, getenv, true, true, true, shell) { t.Fatalf("%s control did not enable ordinary first-run setup", shell) } for _, args := range [][]string{ @@ -80,7 +81,7 @@ func TestLocalUtilityFirstRunSuppression(t *testing.T) { {"openai", "codex", "--destination", "config"}, {"openai", "--format", "json", "help", "tokenizer", "count"}, } { - if imagePickerFirstRunEligible(args, getenv, true, true, true, shell) { + if images.FirstRunEligible(args, getenv, true, true, true, shell) { t.Errorf("%s: local utility allowed automatic shell setup: %q", shell, args) } } diff --git a/pkg/custom/models_policy.go b/pkg/custom/models_policy.go index 1dff447b2..4d139522c 100644 --- a/pkg/custom/models_policy.go +++ b/pkg/custom/models_policy.go @@ -28,8 +28,8 @@ func configureModelsList(root *cli.Command) { if !commandAllowsOutputDiagnostics(command) { return func() {} } - stop, _ := startLoadingFeedback(ctx, os.Stderr, "Loading models", "", loadingAnimationSupported(os.Getenv), - imageLoadingSpinner(os.Getenv, runtime.GOOS), func() (int, int) { + stop, _ := clioutput.StartLoadingFeedback(ctx, os.Stderr, "Loading models", "", clioutput.LoadingAnimationSupported(os.Getenv), + clioutput.LoadingSpinner(os.Getenv, runtime.GOOS), func() (int, int) { width, height, _ := term.GetSize(os.Stderr.Fd()) return width, height }) diff --git a/scripts/check-wsl-pager.py b/scripts/check-wsl-pager.py index de1adf235..919072c2b 100644 --- a/scripts/check-wsl-pager.py +++ b/scripts/check-wsl-pager.py @@ -457,12 +457,12 @@ def unique_object(pairs): raise RuntimeError("Expected a regular WSL command-contract inventory") data = COMMAND_CASES_PATH.read_bytes() suites = json.loads(data, object_pairs_hook=unique_object) - if not isinstance(suites, dict) or set(suites) != {"custom", "process", "app"}: + if not isinstance(suites, dict) or set(suites) != {"custom", "images", "process", "app"}: raise RuntimeError("Invalid WSL command-contract inventory") excluded = set(PROCESS_CASES) | {ROOT, "TestMainDispatchProcess", "TestMainWrappedExitCodeProcess", "TestRequestConfigurationChildEnvironment"} selected = set() - for package, roots, names in (("custom", 10, 71), ("process", 65, 577), ("app", 2, 12)): + for package, roots, names in (("custom", 4, 9), ("images", 6, 62), ("process", 65, 577), ("app", 2, 12)): cases = suites[package] if not isinstance(cases, dict) or len(cases) != roots: raise RuntimeError("Invalid WSL command-contract root inventory") @@ -506,11 +506,11 @@ def run_process_suite(commands, user, target, cases=None): return tests -def pilot(binary, cleanup_only=False, process_binary=None, command_contracts=False, app_binary=None): +def pilot(binary, cleanup_only=False, process_binary=None, command_contracts=False, app_binary=None, images_binary=None): suites = None if command_contracts and not cleanup_only: - if binary is None or process_binary is None or app_binary is None: - raise RuntimeError("Command contracts require all three test binaries") + if binary is None or process_binary is None or app_binary is None or images_binary is None: + raise RuntimeError("Command contracts require all four test binaries") suites, inventory_hash = load_command_cases() # Validate process bitness before selecting System32, which WOW64 can redirect. host_process = require_native_windows_process() @@ -553,6 +553,8 @@ def pilot(binary, cleanup_only=False, process_binary=None, command_contracts=Fal if suites is not None: result["app_binary_sha256"] = require_linux_binary(app_binary) app_binary = app_binary.resolve(strict=True) + result["images_binary_sha256"] = require_linux_binary(images_binary) + images_binary = images_binary.resolve(strict=True) result["wsl_sha256"] = digest(Path(wsl)) result["source"] = commands.run("source", ["git", "rev-parse", "HEAD", "HEAD^{tree}"], 10)[0].decode("ascii").splitlines() result["wsl_version"] = decode_metadata(commands.run("wsl-version", [wsl, "--version"], 20)[0]) @@ -590,6 +592,8 @@ def pilot(binary, cleanup_only=False, process_binary=None, command_contracts=Fal app_target = "/home/cli-compat/payload/app.test" if suites is not None: copy_payload(commands, root, user, app_binary, app_target, result["app_binary_sha256"], "app-") + images_target = "/home/cli-compat/payload/images.test" + copy_payload(commands, root, user, images_binary, images_target, result["images_binary_sha256"], "images-") identity = "set -eu; test $(id -u) = 2000; test $(uname -m) = x86_64; test $(dpkg --print-architecture) = amd64; cat /etc/os-release; uname -a; id; /bin/bash --version; stat -f -c %T /home/cli-compat" observed = commands.run("ubuntu-identity", user + ["/bin/bash", "--noprofile", "--norc", "-c", identity], 20)[0].decode("utf-8") if not re.search(r"^ID=ubuntu$", observed, re.MULTILINE): @@ -607,6 +611,10 @@ def pilot(binary, cleanup_only=False, process_binary=None, command_contracts=Fal if digest(process_binary) != result["process_binary_sha256"]: raise RuntimeError("The host process test binary changed") if suites is not None: + result["images_tests"] = sorted(run_custom_suite(commands, user, images_target, + suites["images"], label="images-contracts")) + if digest(images_binary) != result["images_binary_sha256"]: + raise RuntimeError("The host images test binary changed") result["app_tests"] = sorted(run_custom_suite(commands, user, app_target, suites["app"], label="app-contracts")) if digest(app_binary) != result["app_binary_sha256"]: @@ -646,19 +654,20 @@ def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--binary", type=Path) parser.add_argument("--process-binary", type=Path, help="Also run the fixed noninteractive process suite") + parser.add_argument("--images-binary", type=Path, help="Run the image preference and state contracts") parser.add_argument("--app-binary", type=Path, help="Run the application lifecycle contracts") parser.add_argument("--command-contracts", action="store_true", - help="Run the fixed command-contract supplement using all three test binaries") + help="Run the fixed command-contract supplement using all four test binaries") parser.add_argument("--cleanup", action="store_true") args = parser.parse_args() if not args.cleanup and args.binary is None: parser.error("--binary is required for the pilot") - if args.command_contracts and not args.cleanup and (args.process_binary is None or args.app_binary is None): - parser.error("--command-contracts requires --process-binary and --app-binary") + if args.command_contracts and not args.cleanup and (args.process_binary is None or args.app_binary is None or args.images_binary is None): + parser.error("--command-contracts requires --process-binary, --app-binary, and --images-binary") for number in (signal.SIGINT, signal.SIGTERM): signal.signal(number, interrupted) try: - code = pilot(args.binary, args.cleanup, args.process_binary, args.command_contracts, args.app_binary) + code = pilot(args.binary, args.cleanup, args.process_binary, args.command_contracts, args.app_binary, args.images_binary) except Exception as error: print(f"WSL pager pilot failed: {error}", file=sys.stderr) code = 128 + STOP if STOP else 1 diff --git a/scripts/demos/image_navigation_spacing_record.sh b/scripts/demos/image_navigation_spacing_record.sh index 52798f6de..8177a2dc8 100644 --- a/scripts/demos/image_navigation_spacing_record.sh +++ b/scripts/demos/image_navigation_spacing_record.sh @@ -38,7 +38,7 @@ SCENE "$demo_source/image_navigation_spacing_record.sh" \ "$demo_root/scripts/image-picker-layout/drive_picker.py" \ "$demo_root/scripts/image_picker_harness.py" \ - "$demo_root/pkg/custom/image_picker.go" "$demo_root/pkg/custom/image_picker_view.go" + "$demo_root/internal/commands/images/image_picker.go" "$demo_root/internal/commands/images/image_picker_view.go" } > "$demo_output/metadata.txt" demo_window_size=80x24 diff --git a/scripts/image-picker-layout/build_two_column.py b/scripts/image-picker-layout/build_two_column.py index bcfcfd636..a3d72de7c 100644 --- a/scripts/image-picker-layout/build_two_column.py +++ b/scripts/image-picker-layout/build_two_column.py @@ -10,7 +10,7 @@ parser.add_argument("output", type=Path) args = parser.parse_args() root = Path(__file__).resolve().parents[2] -source = root / "pkg/custom/image_picker_view.go" +source = root / "internal/commands/images/image_picker_view.go" patch = Path(__file__).with_name("two-column.patch") with tempfile.TemporaryDirectory(prefix="image-layout-prototype-") as temporary: directory = Path(temporary) diff --git a/scripts/image-picker-layout/record.sh b/scripts/image-picker-layout/record.sh index 4b5a5669d..2c904ed86 100755 --- a/scripts/image-picker-layout/record.sh +++ b/scripts/image-picker-layout/record.sh @@ -66,7 +66,7 @@ SCENE "$demo_source/prompt_frame.py" \ "$demo_source/build_two_column.py" "$demo_source/two-column.patch" \ "$demo_root/scripts/image_picker_harness.py" \ - "$demo_root/pkg/custom/image_picker.go" "$demo_root/pkg/custom/image_picker_view.go" + "$demo_root/internal/commands/images/image_picker.go" "$demo_root/internal/commands/images/image_picker_view.go" } > "$demo_output/metadata.txt" demo_render_options=(--renderer resvg --text-font-family Menlo --font-size 20 --line-height 1.2 \ diff --git a/scripts/image-picker-layout/two-column.patch b/scripts/image-picker-layout/two-column.patch index bf57acf61..27cccfcb2 100644 --- a/scripts/image-picker-layout/two-column.patch +++ b/scripts/image-picker-layout/two-column.patch @@ -1,13 +1,16 @@ ---- a/pkg/custom/image_picker_view.go -+++ b/pkg/custom/image_picker_view.go -@@ -165,7 +165,20 @@ +--- a/internal/commands/images/image_picker_view.go ++++ b/internal/commands/images/image_picker_view.go +@@ -162,10 +162,20 @@ - for i := start; i < end; i++ { - row := rows[i] - text := row.label - if row.id == "choice" { -- field := m.field -- if row.value == m.value(field) { -- text += " ✓" +- text = ansi.Truncate(text, choiceWidth, "…") +- text += strings.Repeat(" ", choiceWidth-ansi.StringWidth(text)) + " " +- if row.value == m.value(m.field) { +- text += "✓" +- } else { +- text += " " + if m.page == "settings" && m.width >= 76 && available >= len(rows)+1 { + columnWidth := (width - 2) / 2 + for i := 0; i < len(rows); i += 2 { @@ -28,7 +31,7 @@ + label, value = selected.Render(label), selected.Render(value) + } + labels, values = append(labels, label), append(values, value) -@@ -173,2 +186,17 @@ +@@ -173,2 +183,20 @@ - if row.detail != "" { - text += " " + row.detail + lines = append(lines, strings.Join(labels, " ")) @@ -39,9 +42,12 @@ + row := rows[i] + text := row.label + if row.id == "choice" { -+ field := m.field -+ if row.value == m.value(field) { -+ text += " ✓" ++ text = ansi.Truncate(text, choiceWidth, "…") ++ text += strings.Repeat(" ", choiceWidth-ansi.StringWidth(text)) + " " ++ if row.value == m.value(m.field) { ++ text += "✓" ++ } else { ++ text += " " + } + if row.detail != "" { + text += " " + row.detail diff --git a/scripts/test_wsl_pager.py b/scripts/test_wsl_pager.py index 126e42f84..ecb521110 100644 --- a/scripts/test_wsl_pager.py +++ b/scripts/test_wsl_pager.py @@ -361,8 +361,9 @@ def test_fixed_inventory_and_literal_suffixes(self): suites, checksum = pilot.load_command_cases() self.assertEqual(suites, self.suites) self.assertEqual(checksum, hashlib.sha256(self.raw).hexdigest()) - self.assertEqual(set(suites), {"custom", "process", "app"}) - self.assertEqual((len(suites["custom"]), len(self.names(suites["custom"]))), (10, 71)) + self.assertEqual(set(suites), {"custom", "images", "process", "app"}) + self.assertEqual((len(suites["custom"]), len(self.names(suites["custom"]))), (4, 9)) + self.assertEqual((len(suites["images"]), len(self.names(suites["images"]))), (6, 62)) self.assertEqual((len(suites["process"]), len(self.names(suites["process"]))), (65, 577)) self.assertEqual((len(suites["app"]), len(self.names(suites["app"]))), (2, 12)) self.assertEqual(set(suites["app"]), {"TestRequestConfigurationRestoresEnvironment", @@ -371,8 +372,8 @@ def test_fixed_inventory_and_literal_suffixes(self): "placement_precedence_and_error", "root_and_body_project", "equals_empty", "separate_empty", "completion_stays_local", ]) - self.assertIn("absent/", suites["custom"]["TestImageInlinePreferencePrecedence"]) - self.assertIn("absent/#01", suites["custom"]["TestImageInlinePreferencePrecedence"]) + self.assertIn("absent/", suites["images"]["TestImageInlinePreferencePrecedence"]) + self.assertIn("absent/#01", suites["images"]["TestImageInlinePreferencePrecedence"]) self.assertIn("#00", suites["process"]["TestMainGlobalFlagsDeepAdminAndCustomCommands"]) self.assertIn("missing_required_flag/", suites["process"]["TestMainLocalErrorsStayOnStderr"]) self.assertIn('{"error":true}/--format/text/--format-error/json', @@ -452,6 +453,7 @@ def test_inventory_rejects_helper_baseline_and_cross_package_roots(self): def test_supplement_suites_require_exact_output(self): for package, runner in (("custom", pilot.run_custom_suite), ("process", pilot.run_process_suite), + ("images", lambda *args: pilot.run_custom_suite(*args, label="images-contracts")), ("app", lambda *args: pilot.run_custom_suite(*args, label="app-contracts"))): cases = self.suites[package] good = self.output(cases) @@ -518,7 +520,9 @@ def test_process_supplement_preserves_full_admission_and_command(self): def test_cli_rejects_missing_payloads_and_unknown_flags(self): for argv in (["--command-contracts"], ["--command-contracts", "--binary", "custom.test"], ["--command-contracts", "--process-binary", "process.test"], - ["--command-contracts", "--binary", "custom.test", "--process-binary", "process.test"], ["--unknown"]): + ["--command-contracts", "--binary", "custom.test", "--process-binary", "process.test"], + ["--command-contracts", "--binary", "custom.test", "--process-binary", "process.test", + "--app-binary", "app.test"], ["--unknown"]): with self.subTest(argv=argv): with patch.object(pilot.sys, "argv", ["check-wsl-pager.py", *argv]), \ patch.object(pilot.sys, "stderr", io.StringIO()), patch.object(pilot, "pilot") as run: @@ -530,15 +534,15 @@ def test_cli_rejects_missing_payloads_and_unknown_flags(self): def test_cli_preserves_profile_and_cleanup_flags(self): for argv, expected in ( (["--command-contracts", "--binary", "custom.test", "--process-binary", "process.test", - "--app-binary", "app.test"], - (Path("custom.test"), False, Path("process.test"), True, Path("app.test"))), - (["--cleanup", "--command-contracts"], (None, True, None, True, None)), - (["--binary", "custom.test"], (Path("custom.test"), False, None, False, None)), + "--app-binary", "app.test", "--images-binary", "images.test"], + (Path("custom.test"), False, Path("process.test"), True, Path("app.test"), Path("images.test"))), + (["--cleanup", "--command-contracts"], (None, True, None, True, None, None)), + (["--binary", "custom.test"], (Path("custom.test"), False, None, False, None, None)), ): calls = [] - def run(binary, cleanup_only=False, process_binary=None, command_contracts=False, app_binary=None): - calls.append((binary, cleanup_only, process_binary, command_contracts, app_binary)) + def run(binary, cleanup_only=False, process_binary=None, command_contracts=False, app_binary=None, images_binary=None): + calls.append((binary, cleanup_only, process_binary, command_contracts, app_binary, images_binary)) return 0 with self.subTest(argv=argv): @@ -553,7 +557,7 @@ def test_direct_pilot_requires_all_payloads_before_preparation(self): with self.subTest(binary=binary, process=process_binary): with patch.object(pilot, "load_command_cases") as load, \ patch.object(pilot, "require_native_windows_process") as native: - with self.assertRaisesRegex(RuntimeError, "all three test binaries"): + with self.assertRaisesRegex(RuntimeError, "all four test binaries"): pilot.pilot(binary, process_binary=process_binary, command_contracts=True) load.assert_not_called() native.assert_not_called() @@ -583,15 +587,17 @@ def finish(self): with tempfile.TemporaryDirectory() as directory: parent = Path(directory) - custom, process, app = (parent / name for name in ("custom.test", "process.test", "app.test")) + custom, process, app, images = (parent / name for name in ("custom.test", "process.test", "app.test", "images.test")) custom.write_bytes(b"synthetic") process.write_bytes(b"synthetic") app.write_bytes(b"synthetic") + images.write_bytes(b"synthetic") owned = parent / "owned" commands = FakeCommands() custom_names = self.names(self.suites["custom"]) process_names = self.names(self.suites["process"]) app_names = self.names(self.suites["app"]) + images_names = self.names(self.suites["images"]) with patch.object(pilot, "load_command_cases", return_value=(self.suites, "a" * 64)) as load, \ patch.object(pilot, "require_native_windows_process", return_value={}), \ patch.object(pilot, "owned_paths", return_value=("owned", owned)), \ @@ -603,19 +609,24 @@ def finish(self): patch.object(pilot.shutil, "disk_usage", return_value=Mock(free=6 * 1024**3)), \ patch.object(pilot, "download_image", return_value=0), \ patch.object(pilot, "copy_payload") as copy, patch.object(pilot, "clean_distro"), \ - patch.object(pilot, "run_custom_suite", side_effect=[custom_names, app_names]) as run_custom, \ + patch.object(pilot, "run_custom_suite", side_effect=[custom_names, images_names, app_names]) as run_custom, \ patch.object(pilot, "run_process_suite", return_value=process_names) as run_process, \ patch.dict(os.environ, {"SYSTEMROOT": str(parent / "windows")}): - self.assertEqual(pilot.pilot(custom, process_binary=process, command_contracts=True, app_binary=app), 0) + self.assertEqual(pilot.pilot(custom, process_binary=process, command_contracts=True, app_binary=app, images_binary=images), 0) load.assert_called_once_with() - self.assertEqual(run_custom.call_count, 2) + self.assertEqual(run_custom.call_count, 3) run_process.assert_called_once() self.assertEqual(run_custom.call_args_list[0].args[2:], ("/home/cli-compat/payload/custom.test", self.suites["custom"])) self.assertEqual(run_process.call_args.args[2:], ("/home/cli-compat/payload/process.test", self.suites["process"])) - self.assertEqual(run_custom.call_args_list[1].args[2:], ("/home/cli-compat/payload/app.test", self.suites["app"])) - self.assertEqual(run_custom.call_args_list[1].kwargs, {"label": "app-contracts"}) + self.assertEqual(run_custom.call_args_list[2].args[2:], ("/home/cli-compat/payload/app.test", self.suites["app"])) + self.assertEqual(run_custom.call_args_list[2].kwargs, {"label": "app-contracts"}) self.assertEqual(copy.call_args_list[2].args[3:], (app.resolve(), "/home/cli-compat/payload/app.test", "b" * 64, "app-")) + self.assertEqual(run_custom.call_args_list[1].args[2:], ("/home/cli-compat/payload/images.test", self.suites["images"])) + self.assertEqual(run_custom.call_args_list[1].kwargs, {"label": "images-contracts"}) + self.assertEqual(copy.call_args_list[3].args[3:], (images.resolve(), "/home/cli-compat/payload/images.test", "b" * 64, "images-")) result = json.loads((owned / "result.json").read_text()) + self.assertEqual(result["images_tests"], sorted(images_names)) + self.assertEqual(result["images_binary_sha256"], "b" * 64) self.assertEqual(result["selection"], "command-contracts") self.assertEqual(result["command_cases_sha256"], "a" * 64) self.assertEqual(result["tests"], sorted(custom_names)) diff --git a/scripts/wsl-command-cases.json b/scripts/wsl-command-cases.json index 6f13f05ec..dc6ccca2e 100644 --- a/scripts/wsl-command-cases.json +++ b/scripts/wsl-command-cases.json @@ -9,7 +9,9 @@ "duplicate", "validation" ], - "TestMTLSClientFlagEnvironmentDoesNotConsumeExplicitValue": [], + "TestMTLSClientFlagEnvironmentDoesNotConsumeExplicitValue": [] + }, + "images": { "TestImageInlinePreferencePrecedence": [ "absent/", "absent/#01",