From 7852dbbc92f133a9cc8b17ea579a94dbddc40956 Mon Sep 17 00:00:00 2001 From: Braden Ream <51544548+Bradenream@users.noreply.github.com> Date: Thu, 1 Oct 2026 16:55:19 -0400 Subject: [PATCH 1/2] fix: stop --dry-run reporting an API error on commands that return 201 --dry-run swaps the HTTP client for one that prints the request and, instead of sending it, hands the SDK a stand-in 200. The SDK accepts only an operation's documented success code, and 28 of the 191 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 response now carries an X-Vf-Dry-Run header. output.Error, which every generated command reports SDK errors through, does not report an error about that response. Errors raised before the preview, such as a body that fails to serialize, carry no response and are still reported. Commands that already worked are unchanged. Checked against all 191 API commands, run with placeholder flags. Of the ones the placeholders could build, master printed the preview and then failed 23; with this change none fail. The other 5 of the 28 were checked by hand: each failed on master and now exits 0. --- internal/client/diagnostics.go | 10 +++- internal/client/diagnostics_test.go | 29 +++++++++ internal/output/dryrun.go | 25 ++++++++ internal/output/dryrun_test.go | 69 ++++++++++++++++++++++ internal/output/output.go | 4 ++ test/dry-run.test.ts | 92 +++++++++++++++++++++++++++++ 6 files changed, 228 insertions(+), 1 deletion(-) create mode 100644 internal/client/diagnostics_test.go create mode 100644 internal/output/dryrun.go create mode 100644 internal/output/dryrun_test.go create mode 100644 test/dry-run.test.ts 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..8278a382 --- /dev/null +++ b/internal/output/dryrun.go @@ -0,0 +1,25 @@ +// 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. + +package output + +// 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(err error) bool { + 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..1fd4a512 --- /dev/null +++ b/internal/output/dryrun_test.go @@ -0,0 +1,69 @@ +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 and returns what it returned and printed. +func reportError(t *testing.T, err error) (error, string) { + t.Helper() + var stderr bytes.Buffer + cmd := &cobra.Command{Use: "vf"} + 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())) + 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) + + for name, err := range map[string]error{ + "a response the API sent": statusError(http.Header{"Content-Type": {"application/json"}}), + "an error before the preview": errors.New("error serializing request body: boom"), + } { + t.Run(name, func(t *testing.T) { + got, printed := reportError(t, err) + 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..0407497c 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(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..b911f046 --- /dev/null +++ b/test/dry-run.test.ts @@ -0,0 +1,92 @@ +// 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 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; + +beforeAll(() => { + // vf keeps credentials under HOME; an empty one keeps the developer's out. + home = fs.mkdtempSync(path.join(os.tmpdir(), 'vf-dry-run-home-')); +}); + +afterAll(() => { + fs.rmSync(home, { recursive: true, force: true }); +}); + +function dryRun(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, '--dry-run', '--token', 'vfp_x'], + ); +} + +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 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'); + }); +}); From 83bd36a46d54b17ef3c6f50b45e432dd006d0a62 Mon Sep 17 00:00:00 2001 From: Braden Ream <51544548+Bradenream@users.noreply.github.com> Date: Thu, 1 Oct 2026 18:44:57 -0400 Subject: [PATCH 2/2] fix: honour the dry-run marker only while --dry-run is on output.Error skipped any error whose response carried X-Vf-Dry-Run. A header is something any server reached with --server-url can send, so a server could have turned its own error into a silent exit 0, without a dry run ever being asked for. Copilot raised this in review. The marker now counts only while --dry-run is on. During a dry run nothing is sent, so the stand-in is the only response there can be. A test server that answers a real request with the marker and an unexpected status shows the difference: before, vf exited 0; now it reports the error and exits 1. --- internal/output/dryrun.go | 16 ++++++++++++++- internal/output/dryrun_test.go | 33 ++++++++++++++++++++++--------- internal/output/output.go | 2 +- test/dry-run.test.ts | 36 ++++++++++++++++++++++++++++------ 4 files changed, 70 insertions(+), 17 deletions(-) diff --git a/internal/output/dryrun.go b/internal/output/dryrun.go index 8278a382..c05d1efc 100644 --- a/internal/output/dryrun.go +++ b/internal/output/dryrun.go @@ -11,15 +11,29 @@ // 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(err error) bool { +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 index 1fd4a512..2aa3e15d 100644 --- a/internal/output/dryrun_test.go +++ b/internal/output/dryrun_test.go @@ -24,11 +24,18 @@ func standInHeader() http.Header { return header } -// reportError runs Error and returns what it returned and printed. -func reportError(t *testing.T, err error) (error, string) { +// 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() } @@ -43,7 +50,7 @@ func TestErrorIgnoresTheDryRunStandInResponse(t *testing.T) { InitAgentMode(&cobra.Command{Use: "vf"}) } - got, printed := reportError(t, statusError(standInHeader())) + got, printed := reportError(t, statusError(standInHeader()), true) if got != nil || printed != "" { t.Fatalf("Error = %v, printed %q; want nil and nothing printed", got, printed) } @@ -55,12 +62,20 @@ func TestErrorStillReportsEveryOtherError(t *testing.T) { ResetAgentMode() t.Cleanup(ResetAgentMode) - for name, err := range map[string]error{ - "a response the API sent": statusError(http.Header{"Content-Type": {"application/json"}}), - "an error before the preview": errors.New("error serializing request body: boom"), - } { - t.Run(name, func(t *testing.T) { - got, printed := reportError(t, err) + 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 0407497c..9e617b75 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -280,7 +280,7 @@ func Error(cmd *cobra.Command, err error) error { return nil } // A dry run's stand-in response is not an API error. See dryrun.go. - if isAboutDryRunResponse(err) { + if isAboutDryRunResponse(cmd, err) { return nil } diff --git a/test/dry-run.test.ts b/test/dry-run.test.ts index b911f046..3f493cf3 100644 --- a/test/dry-run.test.ts +++ b/test/dry-run.test.ts @@ -12,6 +12,8 @@ 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'; @@ -27,25 +29,37 @@ const AGENT_ENV_VARS = [ ]; let home: string; +let server: http.Server; +let serverURL: string; -beforeAll(() => { +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(() => { +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); fs.rmSync(home, { recursive: true, force: true }); }); -function dryRun(args: string[], opts: { agentMode?: boolean } = {}) { +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, '--dry-run', '--token', 'vfp_x'], - ); + 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 @@ -80,6 +94,16 @@ describe('--dry-run', () => { } }); + // 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 () => {