diff --git a/internal/client/diagnostics.go b/internal/client/diagnostics.go index 8136e648..7f37286e 100644 --- a/internal/client/diagnostics.go +++ b/internal/client/diagnostics.go @@ -14,6 +14,7 @@ import ( "github.com/spf13/cobra" "github.com/voiceflow/cli/internal/flagutil" + "github.com/voiceflow/cli/internal/output" ) // maxBodyPreview is the maximum number of bytes to show in body previews. @@ -244,10 +245,17 @@ func (c *DryRunClient) Do(req *http.Request) (*http.Response, error) { } fmt.Fprintf(c.Stderr, "[DRY-RUN] Network call skipped.\n") + // The marker tells output.Error that an SDK error about this response + // (operations that succeed only with 201 reject a 200) is not an API + // error. See internal/output/dryrun.go. + header := http.Header{} + header.Set("Content-Type", "application/json") + header.Set(output.DryRunResponseHeader, "true") + return &http.Response{ StatusCode: http.StatusOK, Status: "200 OK", - Header: http.Header{"Content-Type": []string{"application/json"}}, + Header: header, Body: io.NopCloser(bytes.NewReader([]byte("{}"))), Request: req, }, nil diff --git a/internal/client/diagnostics_test.go b/internal/client/diagnostics_test.go new file mode 100644 index 00000000..584fde05 --- /dev/null +++ b/internal/client/diagnostics_test.go @@ -0,0 +1,29 @@ +package client + +import ( + "bytes" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/voiceflow/cli/internal/output" +) + +// output.Error recognizes the stand-in response by this marker, so a dry run +// of an operation that succeeds only with 201 does not report an API error. +func TestDryRunClientMarksItsStandInResponse(t *testing.T) { + var stderr bytes.Buffer + req := httptest.NewRequest(http.MethodPost, "https://api.example.com/v1/stable/project", strings.NewReader(`{"name":"n"}`)) + + res, err := (&DryRunClient{Stderr: &stderr}).Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + if res.Header.Get(output.DryRunResponseHeader) == "" { + t.Errorf("the stand-in response is not marked with %s", output.DryRunResponseHeader) + } + if !strings.Contains(stderr.String(), "[DRY-RUN] Network call skipped.") { + t.Errorf("no request preview printed:\n%s", stderr.String()) + } +} diff --git a/internal/output/dryrun.go b/internal/output/dryrun.go new file mode 100644 index 00000000..c05d1efc --- /dev/null +++ b/internal/output/dryrun.go @@ -0,0 +1,39 @@ +// This file is not generated by Speakeasy. It stops a dry run from reporting an +// API error about a response the API never sent. +// +// --dry-run swaps the HTTP client for one that prints the request and, instead +// of sending it, hands the SDK a stand-in response: 200 with an empty JSON +// body. The SDK accepts only an operation's documented success code, and 28 +// operations succeed with 201 alone: every create, plus environment clone and +// publish, evaluation run and test run create. Their dry runs printed the +// preview, then "API Error (HTTP 200): unknown status code returned", and +// exited 1. The stand-in carries DryRunResponseHeader, and Error does not +// report an error about it: the preview has printed and nothing was sent. +// Errors raised before the preview, such as a body that fails to serialize, +// carry no response and are still reported. +// +// The marker counts only while --dry-run is on. A header is something any +// server reached with --server-url could send, and it must not be able to turn +// its own error into a silent success. During a dry run nothing is sent, so +// the stand-in is the only response there can be. + +package output + +import ( + "github.com/spf13/cobra" + + "github.com/voiceflow/cli/internal/flagutil" +) + +// DryRunResponseHeader marks the stand-in response that a dry run returns in +// place of sending the request. +const DryRunResponseHeader = "X-Vf-Dry-Run" + +// isAboutDryRunResponse reports whether err concerns a dry run's stand-in +// response rather than anything the API returned. +func isAboutDryRunResponse(cmd *cobra.Command, err error) bool { + if isDryRun, _ := flagutil.GetBoolFlag(cmd, "dry-run"); !isDryRun { + return false + } + return extractErrorResponseHeaders(err).Get(DryRunResponseHeader) != "" +} diff --git a/internal/output/dryrun_test.go b/internal/output/dryrun_test.go new file mode 100644 index 00000000..2aa3e15d --- /dev/null +++ b/internal/output/dryrun_test.go @@ -0,0 +1,84 @@ +package output + +import ( + "bytes" + "errors" + "net/http" + "testing" + + "github.com/spf13/cobra" + "github.com/voiceflow/cli/internal/sdk/models/sdkerrors" +) + +// statusError is what the SDK returns for a response whose status code the +// operation does not expect, as a 201-only operation does for a dry run's 200. +func statusError(header http.Header) error { + res := &http.Response{StatusCode: http.StatusOK, Status: "200 OK", Header: header} + return sdkerrors.NewSDKDefaultError("unknown status code returned", http.StatusOK, "{}", res) +} + +func standInHeader() http.Header { + header := http.Header{} + header.Set("Content-Type", "application/json") + header.Set(DryRunResponseHeader, "true") + return header +} + +// reportError runs Error, with or without --dry-run, and returns what it +// returned and printed. +func reportError(t *testing.T, err error, isDryRun bool) (error, string) { + t.Helper() + var stderr bytes.Buffer + cmd := &cobra.Command{Use: "vf"} + cmd.Flags().Bool("dry-run", false, "") + if isDryRun { + if setErr := cmd.Flags().Set("dry-run", "true"); setErr != nil { + t.Fatal(setErr) + } + } + cmd.SetErr(&stderr) + return Error(cmd, err), stderr.String() +} + +func TestErrorIgnoresTheDryRunStandInResponse(t *testing.T) { + for name, isAgentMode := range map[string]bool{"human mode": false, "agent mode": true} { + t.Run(name, func(t *testing.T) { + ResetAgentMode() + t.Cleanup(ResetAgentMode) + if isAgentMode { + t.Setenv("FORCE_AGENT_MODE", "1") + InitAgentMode(&cobra.Command{Use: "vf"}) + } + + got, printed := reportError(t, statusError(standInHeader()), true) + if got != nil || printed != "" { + t.Fatalf("Error = %v, printed %q; want nil and nothing printed", got, printed) + } + }) + } +} + +func TestErrorStillReportsEveryOtherError(t *testing.T) { + ResetAgentMode() + t.Cleanup(ResetAgentMode) + + cases := []struct { + name string + err error + isDryRun bool + }{ + {"a response the API sent", statusError(http.Header{"Content-Type": {"application/json"}}), true}, + {"an error before the preview", errors.New("error serializing request body: boom"), true}, + // The marker is a header any server could send; outside a dry run it + // must not turn that server's error into a success. + {"a marked response without --dry-run", statusError(standInHeader()), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, printed := reportError(t, tc.err, tc.isDryRun) + if got == nil || printed == "" { + t.Fatalf("Error = %v, printed %q; want the error returned and reported", got, printed) + } + }) + } +} diff --git a/internal/output/output.go b/internal/output/output.go index 8763109d..9e617b75 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -279,6 +279,10 @@ func Error(cmd *cobra.Command, err error) error { if err == nil { return nil } + // A dry run's stand-in response is not an API error. See dryrun.go. + if isAboutDryRunResponse(cmd, err) { + return nil + } format := resolveOutputFormat(cmd) jqExpr, _ := flagutil.GetStringFlag(cmd, "jq") diff --git a/test/dry-run.test.ts b/test/dry-run.test.ts new file mode 100644 index 00000000..3f493cf3 --- /dev/null +++ b/test/dry-run.test.ts @@ -0,0 +1,116 @@ +// Tests for --dry-run (internal/client/diagnostics.go, internal/output/dryrun.go): +// it prints the request it would send and exits 0, whatever status code the +// real operation succeeds with. Nothing reaches the network. +// +// A dry run hands the SDK a stand-in 200 instead of sending the request, and +// the SDK accepts only an operation's documented success code. The 28 +// operations that succeed only with 201 (every create, plus environment clone +// and publish, evaluation run and test run create) used to print the preview, +// then "API Error (HTTP 200): unknown status code returned", and exit 1. +// +// Requires: go build -o vf ./cmd/vf + +import { execa } from 'execa'; +import * as fs from 'node:fs'; +import * as http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +const VF = path.resolve(__dirname, '..', 'vf'); + +// Every variable that puts the CLI into agent mode. Mirrors the list in +// internal/output/agentmode.go; a stray one on the host would flip the renderer. +const AGENT_ENV_VARS = [ + 'CLAUDECODE', 'CLAUDE_CODE', 'CURSOR_AGENT', 'CODEX', 'AIDER', 'CLINE', + 'WINDSURF_AGENT', 'GITHUB_COPILOT', 'AMAZON_Q', 'GEMINI_CODE_ASSIST', + 'SRC_CODY', 'FORCE_AGENT_MODE', +]; + +let home: string; +let server: http.Server; +let serverURL: string; + +beforeAll(async () => { + // vf keeps credentials under HOME; an empty one keeps the developer's out. + home = fs.mkdtempSync(path.join(os.tmpdir(), 'vf-dry-run-home-')); + + // A real server that answers with the dry-run marker, as any server reached + // with --server-url could. + server = http.createServer((_req, res) => { + res.writeHead(200, { 'content-type': 'application/json', 'x-vf-dry-run': 'true' }); + res.end('{}'); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + serverURL = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + fs.rmSync(home, { recursive: true, force: true }); +}); + +function vf(args: string[], opts: { agentMode?: boolean } = {}) { + const env: Record = Object.fromEntries(AGENT_ENV_VARS.map((name) => [name, undefined])); + env.HOME = home; + if (opts.agentMode) env.CLAUDECODE = '1'; + return execa({ reject: false, timeout: 20_000, stdin: 'ignore', env, extendEnv: true })(VF, [...args, '--token', 'vfp_x']); +} + +const dryRun = (args: string[], opts: { agentMode?: boolean } = {}) => vf([...args, '--dry-run'], opts); + +const PREVIEWED = '[DRY-RUN] Network call skipped.'; + +// Operations that succeed only with 201, and one that succeeds with 200, which +// always worked. +const COMMANDS: Array<[name: string, args: string[]]> = [ + ['project create', ['project', 'create', '--name', 'n', '--type', 'webchat', '--workspace-id', 'w']], + ['environment publish', ['environment', 'publish', '--project-id', 'p', '--environment-alias', 'main', '--name', 'v1']], + ['agent update', ['agent', 'update', '--project-id', 'p', '--environment-alias', 'main', '--prompt', 'x']], +]; + +describe('--dry-run', () => { + for (const [name, args] of COMMANDS) { + it(`previews vf ${name} and exits 0`, async () => { + const result = await dryRun(args); + + expect(result.exitCode, result.stderr).toBe(0); + expect(result.stderr).toContain(PREVIEWED); + expect(result.stderr).not.toContain('API Error'); + expect(result.stderr).not.toContain('unknown status code'); + }); + } + + it('reports no error in agent mode or with --output-format json either', async () => { + const [, createProject] = COMMANDS[0]!; + for (const result of [ + await dryRun(createProject, { agentMode: true }), + await dryRun([...createProject, '--output-format', 'json']), + ]) { + expect(result.exitCode, result.stderr).toBe(0); + expect(result.stderr).toContain(PREVIEWED); + expect(result.stderr).not.toContain('"error'); + } + }); + + // The marker is only honoured during a dry run. Otherwise a server could send + // it on an error response and turn the error into a silent success. + it('still reports an unexpected response that carries the marker without --dry-run', async () => { + const [, createProject] = COMMANDS[0]!; + const result = await vf([...createProject, '--server-url', serverURL]); + + expect(result.exitCode, result.stderr).toBe(1); + expect(result.stderr).toContain('unknown status code'); + }); + + // The rule must not hide real failures: a request that cannot be built fails + // before anything is previewed, and is still an error. + it('still fails when the request cannot be built', async () => { + const result = await dryRun(['tool', 'create', '--project-id', 'p', '--environment-alias', 'main']); + + expect(result.exitCode).toBe(1); + expect(result.stderr).not.toContain(PREVIEWED); + expect(result.stderr).toContain('error serializing request body'); + }); +});