-
Notifications
You must be signed in to change notification settings - Fork 0
fix: stop --dry-run reporting an API error on commands that return 201 #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) != "" | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| } | ||
| }) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<void>((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<void>((resolve) => server.close(() => resolve())); | ||
| fs.rmSync(home, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| function vf(args: string[], opts: { agentMode?: boolean } = {}) { | ||
| const env: Record<string, string | undefined> = 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'); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.