From 41fa0a33389d6502c3712394f0c98889f40a322f Mon Sep 17 00:00:00 2001 From: Bradenream <51544548+Bradenream@users.noreply.github.com> Date: Fri, 2 Oct 2026 15:30:24 +0000 Subject: [PATCH] fix: restore vf docs search, and make the test suite hermetic and green (#37) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Two problems made the behaviour suite untrustworthy, and one of them hid a real outage. **1. The suite ran against the developer's own HOME.** Every `vf` it spawned read `~/.config/vf` and, on macOS, the login Keychain. The no-token tests found a real session and failed. Worse, a run refreshed the real OAuth session: it rewrote `oauth.json` and rotated the tokens in the Keychain. `test/setup.ts` now gives each test file an empty temp HOME, and cuts off the OS keyring on both platforms the suite runs on: - **macOS:** under the empty HOME, `/usr/bin/security` has no default keychain. - **Linux:** go-keyring reaches the Secret Service over D-Bus, which HOME does not affect. So setup also points `DBUS_SESSION_BUS_ADDRESS` at a socket that does not exist. Either way, vf can neither read nor overwrite the developer's tokens. The integration tests still authenticate with `VF_TOKEN` from `.env.test`. **2. Four tests were red on master** (`docs-command` ×2, `flag-errors`, `flag-raw-text`), for three different reasons: - **`vf docs search` is broken for every user.** The docs MCP server renamed `search_voiceflow_documentation` to `search_voiceflow`, which takes `question`, `initiator`, `purpose` and `why`, and returns `structuredContent`. The server reports an unknown tool inside a successful result (`isError: true`), and vf never checked that flag: it printed "no such tool" as a search hit and exited 0. vf now calls the new tool and keeps the same `{title, link, page, content}` output. A tool error is now an error. - **The help-text check went stale.** A regeneration changed the descriptions of the two flags it named to "string value", removing their backticks. Their two cases kept passing with nothing to check, and the prose case failed. The check now takes every flag whose description has a backtick from `vf --usage` (16 today) and checks each one's `--help`. A separate case fails if it finds none. - **The Markup strictness check lost its flag.** A regeneration made `mcp-server create --url` a plain string and removed `Markup` from the spec. The check now uses `transcript search --filters`, a list of unions: the shape `--url` used to have. A reflection probe over all 90 JSON flags found none that is a union with a string member outside a list. So a unit test now pins `targetsStringValue` on its own types, which the spec cannot change. ## Changes from review - **Shell variables:** the suite now takes its `VF_*` settings from `.env.test` alone (`test/env.ts`). `VF_SKIP_INTEGRATION_TESTS` is the one exception. Before, a `VF_TOKEN` and `VF_WORKSPACE_ID` exported in the developer's shell queued all 16 integration test files against that account. - **Credential guard:** `test/isolation.test.ts` fails if a vf spawned by the suite finds a token from any source, an OAuth session, or a config file in the real home. - **`vf docs search`:** - finds the reply anywhere in the server's event stream; - gives a reason for a tool error that has no text; - takes a hit's page from its link when `pageUrl` is missing; - prints `[]` for no matches in JSON output, and applies `--jq`. - **Help-text check:** each flag is checked against its own help entry, and its label must be a pflag type name. The `--help` runs go in parallel, and KDL escapes are decoded properly. - **`--filters` strictness check:** it now tells the raw-text fallback apart from a decode failure. ## Before and after | | master | this PR | |---|---|---| | Behaviour suite, empty HOME | 4 failed, 76 passed | 79 passed | | Same, against a decoy HOME holding a token and an OAuth session, with VF_TOKEN and VF_OUTPUT_FORMAT exported in the shell | 9 failed; vf wrote into the decoy | 79 passed; decoy byte-for-byte unchanged | | `vf docs search "personal access token"` | prints "no such tool…", exit 0 | 5 results, exit 0 | | Help-text check with backtick neutralization removed | would catch 1 flag | catches all 16 | ## Test plan - [x] `gofmt`, `go vet ./...` and `go test ./...` pass; `go.mod` is unchanged. - [x] New Go tests: - `internal/cli/docs_test.go` covers parsing, `isError`, page paths and the tool's required arguments. - `internal/flagutil/stringvalue_test.go` covers the raw-text type rule. Loosening the rule fails 4 of its cases. - [x] Full behaviour suite passes 79/79 on current master (`6df4604`). Together with #38–#42 it passes 103/103, with a shell `VF_TOKEN` exported and the decoy HOME unchanged. - [x] Run against the decoy HOME, which is unchanged afterwards. - [x] Live checks of `vf docs search` in human, JSON and agent mode. Each hit's `page` works with `vf docs get`. - [ ] CI The `docs-command` tests hit the live docs site, so they will catch the next contract change too. --- internal/cli/docs.go | 217 +++++++++++++++++++------- internal/cli/docs_test.go | 183 ++++++++++++++++++++++ internal/flagutil/stringvalue_test.go | 47 ++++++ test/env.ts | 24 +++ test/flag-errors.test.ts | 138 +++++++++++++--- test/flag-raw-text.test.ts | 32 ++-- test/isolation.test.ts | 32 ++++ test/setup.ts | 35 ++++- vitest.config.mts | 7 +- 9 files changed, 628 insertions(+), 87 deletions(-) create mode 100644 internal/cli/docs_test.go create mode 100644 internal/flagutil/stringvalue_test.go create mode 100644 test/env.ts create mode 100644 test/isolation.test.ts diff --git a/internal/cli/docs.go b/internal/cli/docs.go index 6cca47e1..da94d192 100644 --- a/internal/cli/docs.go +++ b/internal/cli/docs.go @@ -20,14 +20,19 @@ import ( "time" "github.com/spf13/cobra" + "github.com/voiceflow/cli/internal/flagutil" "github.com/voiceflow/cli/internal/output" ) const ( docsBaseURL = "https://www.voiceflow.com/docs" docsSearchMCPURL = "https://www.voiceflow.com/docs/mcp" - docsSearchTool = "search_voiceflow_documentation" + docsSearchTool = "search_voiceflow" docsClientTimeout = 15 * time.Second + + // docsSearchWhy is the one-sentence goal the search tool requires with + // every call. vf cannot know the user's task, so it names its own. + docsSearchWhy = "Look up Voiceflow documentation from the vf CLI" ) // initDocsCmd registers the docs command group. @@ -74,7 +79,7 @@ voiceflow.com/docs URL.`, }) } -// docsSearchResult is one parsed search hit. +// docsSearchResult is one search hit as vf prints it. type docsSearchResult struct { Title string `json:"title"` Link string `json:"link"` @@ -82,6 +87,14 @@ type docsSearchResult struct { Content string `json:"content"` } +// docsSearchHit is one entry of the search tool's structuredContent.results. +type docsSearchHit struct { + Title string `json:"title"` + URL string `json:"url"` + PageURL string `json:"pageUrl"` + Snippet string `json:"snippet"` +} + // runDocsSearchCmd executes docs search via the public documentation search // endpoint (a stateless JSON-RPC tool call). func runDocsSearchCmd(cmd *cobra.Command, args []string) error { @@ -93,7 +106,7 @@ func runDocsSearchCmd(cmd *cobra.Command, args []string) error { "method": "tools/call", "params": map[string]interface{}{ "name": docsSearchTool, - "arguments": map[string]string{"query": query}, + "arguments": docsSearchArguments(query), }, }) if err != nil { @@ -112,39 +125,73 @@ func runDocsSearchCmd(cmd *cobra.Command, args []string) error { return err } - blocks, err := parseDocsSearchResponse(body) + results, err := parseDocsSearchResponse(body) if err != nil { return err } - if len(blocks) == 0 { + if output.WantsRawJSON(cmd) { + return writeDocsSearchJSON(cmd, results) + } + if len(results) == 0 { fmt.Fprintf(cmd.OutOrStdout(), "No documentation matches for %q. Browse everything at %s\n", query, docsBaseURL) return nil } - if output.WantsRawJSON(cmd) { - results := make([]docsSearchResult, 0, len(blocks)) - for _, block := range blocks { - results = append(results, parseDocsSearchBlock(block)) + out := cmd.OutOrStdout() + for i, result := range results { + if i > 0 { + fmt.Fprint(out, "\n---\n\n") } + fmt.Fprintf(out, "Title: %s\nLink: %s\nPage: %s\nContent: %s\n", result.Title, result.Link, result.Page, result.Content) + } + fmt.Fprintln(out, "\nRead a full page with: vf docs get ") + return nil +} + +// writeDocsSearchJSON prints results as JSON, through the --jq expression when +// one is set, the way other commands apply it. No matches print as [], so a +// caller that parses stdout always gets JSON. +func writeDocsSearchJSON(cmd *cobra.Command, results []docsSearchResult) error { + out := cmd.OutOrStdout() + expression, _ := flagutil.GetStringFlag(cmd, "jq") + if expression == "" { encoded, err := json.MarshalIndent(results, "", " ") if err != nil { return err } - fmt.Fprintln(cmd.OutOrStdout(), string(encoded)) + fmt.Fprintln(out, string(encoded)) return nil } - - out := cmd.OutOrStdout() - for i, block := range blocks { - if i > 0 { - fmt.Fprint(out, "\n---\n\n") + filtered, err := output.ApplyJqFilter(results, expression) + if err != nil { + return err + } + for _, value := range filtered { + encoded, err := json.MarshalIndent(value, "", " ") + if err != nil { + return err } - fmt.Fprintln(out, strings.TrimSpace(block)) + fmt.Fprintln(out, string(encoded)) } - fmt.Fprintln(out, "\nRead a full page with: vf docs get ") return nil } +// docsSearchArguments builds the search tool's arguments. Besides the query, +// the tool requires who is asking and why: an agent that runs vf chose to +// search on its own, so agent mode reports "agent", and anything else "human". +func docsSearchArguments(query string) map[string]string { + initiator := "human" + if output.IsAgentMode() { + initiator = "agent" + } + return map[string]string{ + "initiator": initiator, + "purpose": "answers", + "why": docsSearchWhy, + "question": query, + } +} + // runDocsGetCmd fetches one page's markdown rendition and prints it. func runDocsGetCmd(cmd *cobra.Command, args []string) error { pageURL, err := docsPageURL(args[0]) @@ -232,22 +279,12 @@ func doDocsRequest(request *http.Request) ([]byte, error) { return body, nil } -// parseDocsSearchResponse extracts the text blocks from a JSON-RPC tools/call +// parseDocsSearchResponse extracts the search hits from a JSON-RPC tools/call // response, handling both plain JSON and SSE-framed ("data: {...}") bodies. -func parseDocsSearchResponse(body []byte) ([]string, error) { - payload := body - if !bytes.HasPrefix(bytes.TrimSpace(body), []byte("{")) { - // SSE framing: use the first "data:" line. - payload = nil - for _, line := range bytes.Split(body, []byte("\n")) { - if data, found := bytes.CutPrefix(bytes.TrimSpace(line), []byte("data:")); found { - payload = bytes.TrimSpace(data) - break - } - } - if payload == nil { - return nil, fmt.Errorf("unexpected response from the documentation search endpoint") - } +func parseDocsSearchResponse(body []byte) ([]docsSearchResult, error) { + payload, err := docsSearchResponsePayload(body) + if err != nil { + return nil, err } var response struct { @@ -256,6 +293,10 @@ func parseDocsSearchResponse(body []byte) ([]string, error) { Type string `json:"type"` Text string `json:"text"` } `json:"content"` + StructuredContent *struct { + Results []docsSearchHit `json:"results"` + } `json:"structuredContent"` + IsError bool `json:"isError"` } `json:"result"` Error *struct { Message string `json:"message"` @@ -267,33 +308,101 @@ func parseDocsSearchResponse(body []byte) ([]string, error) { if response.Error != nil { return nil, fmt.Errorf("documentation search failed: %s", response.Error.Message) } + // A tool that fails says so inside a successful JSON-RPC result. Read + // without this check, the failure text printed as if it were a search hit + // and the command exited 0. + if response.Result.IsError { + var reasons []string + for _, content := range response.Result.Content { + if content.Type == "text" && strings.TrimSpace(content.Text) != "" { + reasons = append(reasons, strings.TrimSpace(content.Text)) + } + } + if len(reasons) == 0 { + reasons = []string{"the search tool reported an error without saying why"} + } + return nil, fmt.Errorf("documentation search failed: %s — the docs are also at %s", strings.Join(reasons, "; "), docsBaseURL) + } + if response.Result.StructuredContent == nil { + return nil, fmt.Errorf("unexpected response from the documentation search endpoint: no structured results — the docs are also at %s", docsBaseURL) + } - blocks := make([]string, 0, len(response.Result.Content)) - for _, content := range response.Result.Content { - if content.Type == "text" && strings.TrimSpace(content.Text) != "" { - blocks = append(blocks, content.Text) + hits := response.Result.StructuredContent.Results + results := make([]docsSearchResult, 0, len(hits)) + for _, hit := range hits { + // The page comes from the hit's link when it has no page URL of its + // own; docsPagePath drops the link's #section. + page := hit.PageURL + if page == "" { + page = hit.URL } + results = append(results, docsSearchResult{ + Title: hit.Title, + Link: hit.URL, + Page: docsPagePath(page), + Content: hit.Snippet, + }) } - return blocks, nil + return results, nil } -// parseDocsSearchBlock splits one "Title:/Link:/Page:/Content:" text block -// into a structured result. -func parseDocsSearchBlock(block string) docsSearchResult { - result := docsSearchResult{} - lines := strings.Split(block, "\n") - for i, line := range lines { - switch { - case strings.HasPrefix(line, "Title: "): - result.Title = strings.TrimPrefix(line, "Title: ") - case strings.HasPrefix(line, "Link: "): - result.Link = strings.TrimPrefix(line, "Link: ") - case strings.HasPrefix(line, "Page: "): - result.Page = strings.TrimPrefix(line, "Page: ") - case strings.HasPrefix(line, "Content: "): - result.Content = strings.TrimSpace(strings.TrimPrefix(strings.Join(lines[i:], "\n"), "Content: ")) - return result +// docsSearchResponsePayload returns the JSON-RPC response in body, which is +// either one JSON object or a stream of server-sent events. A stream may carry +// other messages before the response, such as progress notifications, and may +// split one message across several data: lines. The response is the first +// message that carries a result or an error; notifications carry neither. +func docsSearchResponsePayload(body []byte) ([]byte, error) { + trimmed := bytes.TrimSpace(body) + if bytes.HasPrefix(trimmed, []byte("{")) { + return trimmed, nil + } + for _, event := range serverSentEventData(body) { + var message struct { + Result json.RawMessage `json:"result"` + Error json.RawMessage `json:"error"` + } + if json.Unmarshal(event, &message) == nil && (message.Result != nil || message.Error != nil) { + return event, nil } } - return result + return nil, fmt.Errorf("unexpected response from the documentation search endpoint: it held no reply to the search — the docs are also at %s", docsBaseURL) +} + +// serverSentEventData returns the data of each event in an SSE stream: its +// data: lines joined by newlines, as the format defines. +func serverSentEventData(body []byte) [][]byte { + var events, lines [][]byte + endEvent := func() { + if len(lines) > 0 { + events = append(events, bytes.Join(lines, []byte("\n"))) + lines = nil + } + } + for _, line := range bytes.Split(body, []byte("\n")) { + line = bytes.TrimSuffix(line, []byte("\r")) + if len(line) == 0 { + endEvent() + continue + } + if data, isData := bytes.CutPrefix(line, []byte("data:")); isData { + lines = append(lines, bytes.TrimPrefix(data, []byte(" "))) + } + } + endEvent() + return events +} + +// docsPagePath turns a hit's page URL into the argument 'vf docs get' takes: +// https://www.voiceflow.com/docs/api-reference/authentication becomes +// api-reference/authentication. Any other URL is returned whole, and 'vf docs +// get' decides whether it can fetch it. +func docsPagePath(pageURL string) string { + page, isDocsPage := strings.CutPrefix(pageURL, docsBaseURL+"/") + if !isDocsPage { + return pageURL + } + if end := strings.IndexAny(page, "?#"); end >= 0 { + page = page[:end] + } + return page } diff --git a/internal/cli/docs_test.go b/internal/cli/docs_test.go new file mode 100644 index 00000000..51d663bd --- /dev/null +++ b/internal/cli/docs_test.go @@ -0,0 +1,183 @@ +package cli + +import ( + "bytes" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/voiceflow/cli/internal/output" +) + +// A trimmed search_voiceflow response, SSE-framed the way the docs server +// sends it. +const docsSearchSSE = `event: message +data: {"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"Voiceflow — how_to · 1 result(s)"}],"structuredContent":{"intent":"how_to","results":[{"rank":1,"title":"Personal access tokens","trail":"APIs › Getting started › Personal access tokens › Create a token","url":"https://www.voiceflow.com/docs/api-reference/authentication#create-a-token","pageUrl":"https://www.voiceflow.com/docs/api-reference/authentication","snippet":"Open Settings → Access tokens.","why":"glossary","score":2.9}]}}} +` + +func TestParseDocsSearchResponseReadsTheStructuredResults(t *testing.T) { + for name, body := range map[string]string{ + "SSE-framed": docsSearchSSE, + "plain JSON": strings.TrimPrefix(strings.SplitN(docsSearchSSE, "\n", 2)[1], "data: "), + } { + t.Run(name, func(t *testing.T) { + results, err := parseDocsSearchResponse([]byte(body)) + if err != nil { + t.Fatalf("parse: %v", err) + } + want := docsSearchResult{ + Title: "Personal access tokens", + Link: "https://www.voiceflow.com/docs/api-reference/authentication#create-a-token", + Page: "api-reference/authentication", + Content: "Open Settings → Access tokens.", + } + if len(results) != 1 || results[0] != want { + t.Fatalf("results = %+v, want [%+v]", results, want) + } + }) + } +} + +// The response is the message that carries a result. An SSE stream may send +// other messages first, and may split one message across data: lines. +func TestParseDocsSearchResponseFindsTheResultInAStream(t *testing.T) { + result := strings.TrimPrefix(strings.SplitN(docsSearchSSE, "\n", 2)[1], "data: ") + cut := strings.Index(result, `,"structuredContent"`) + for name, body := range map[string]string{ + "after a progress notification": "event: message\n" + + `data: {"jsonrpc":"2.0","method":"notifications/progress","params":{"progress":1}}` + "\n\n" + + docsSearchSSE, + "split across data lines": "event: message\r\ndata: " + result[:cut] + "\r\ndata: " + result[cut:] + "\r\n\r\n", + } { + t.Run(name, func(t *testing.T) { + results, err := parseDocsSearchResponse([]byte(body)) + if err != nil || len(results) != 1 || results[0].Page != "api-reference/authentication" { + t.Fatalf("results = %+v, err = %v; want the one hit", results, err) + } + }) + } +} + +func TestParseDocsSearchResponseFailsOnAStreamWithNoReply(t *testing.T) { + body := "event: message\ndata: {\"jsonrpc\":\"2.0\",\"method\":\"notifications/progress\"}\n\n" + if results, err := parseDocsSearchResponse([]byte(body)); err == nil { + t.Fatalf("parse succeeded with %+v, want an error", results) + } +} + +// The docs server reports a failed tool call inside a successful JSON-RPC +// result. vf used to print that failure as a search hit and exit 0. +func TestParseDocsSearchResponseFailsOnAToolError(t *testing.T) { + body := `{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"no such tool: search_voiceflow_documentation"}],"isError":true}}` + + results, err := parseDocsSearchResponse([]byte(body)) + if err == nil { + t.Fatalf("parse succeeded with %+v, want an error", results) + } + if !strings.Contains(err.Error(), "no such tool: search_voiceflow_documentation") { + t.Errorf("error %q does not carry the server's reason", err) + } +} + +func TestParseDocsSearchResponseNamesAReasonForAToolErrorWithoutText(t *testing.T) { + body := `{"jsonrpc":"2.0","id":1,"result":{"content":[],"isError":true}}` + + _, err := parseDocsSearchResponse([]byte(body)) + if err == nil || !strings.Contains(err.Error(), "without saying why") { + t.Fatalf("err = %v, want one that says the tool gave no reason", err) + } +} + +func TestParseDocsSearchResponseTakesThePageFromTheLinkWhenThereIsNoPageURL(t *testing.T) { + body := `{"jsonrpc":"2.0","id":1,"result":{"structuredContent":{"results":[` + + `{"title":"Personal access tokens","url":"https://www.voiceflow.com/docs/api-reference/authentication#create-a-token","snippet":"s"}]}}}` + + results, err := parseDocsSearchResponse([]byte(body)) + if err != nil || len(results) != 1 || results[0].Page != "api-reference/authentication" { + t.Fatalf("results = %+v, err = %v; want the page taken from the link", results, err) + } +} + +func TestParseDocsSearchResponseFailsWithoutResults(t *testing.T) { + for name, body := range map[string]string{ + "JSON-RPC error": `{"jsonrpc":"2.0","id":1,"error":{"code":-32602,"message":"invalid params"}}`, + "no structured content": `{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"Title: old format"}]}}`, + "not JSON": "event: message\n", + } { + t.Run(name, func(t *testing.T) { + if results, err := parseDocsSearchResponse([]byte(body)); err == nil { + t.Fatalf("parse succeeded with %+v, want an error", results) + } + }) + } +} + +// writeJSON runs writeDocsSearchJSON with --jq set to expression, or unset +// when it is empty, and returns what it printed. +func writeJSON(t *testing.T, results []docsSearchResult, expression string) string { + t.Helper() + var stdout bytes.Buffer + cmd := &cobra.Command{Use: "search"} + cmd.Flags().String("jq", "", "") + if expression != "" { + if err := cmd.Flags().Set("jq", expression); err != nil { + t.Fatal(err) + } + } + cmd.SetOut(&stdout) + if err := writeDocsSearchJSON(cmd, results); err != nil { + t.Fatalf("writeDocsSearchJSON: %v", err) + } + return stdout.String() +} + +func TestDocsSearchJSONPrintsAnEmptyListForNoMatches(t *testing.T) { + if got := writeJSON(t, []docsSearchResult{}, ""); strings.TrimSpace(got) != "[]" { + t.Fatalf("printed %q, want []", got) + } +} + +func TestDocsSearchJSONAppliesJq(t *testing.T) { + results := []docsSearchResult{{Title: "Personal access tokens", Page: "api-reference/authentication"}} + + if got := writeJSON(t, results, ".[0].page"); strings.TrimSpace(got) != `"api-reference/authentication"` { + t.Fatalf("printed %q, want only the page", got) + } + if got := writeJSON(t, results, ""); !strings.Contains(got, `"title": "Personal access tokens"`) { + t.Fatalf("printed %q, want the whole list", got) + } +} + +func TestDocsPagePath(t *testing.T) { + for pageURL, want := range map[string]string{ + "https://www.voiceflow.com/docs/api-reference/authentication": "api-reference/authentication", + "https://www.voiceflow.com/docs/cli/commands/environment/publish#usage": "cli/commands/environment/publish", + "https://www.voiceflow.com/docs/cli/overview?ref=search": "cli/overview", + "https://example.com/docs/page": "https://example.com/docs/page", + } { + if got := docsPagePath(pageURL); got != want { + t.Errorf("docsPagePath(%q) = %q, want %q", pageURL, got, want) + } + } +} + +func TestDocsSearchArgumentsNameWhoIsAsking(t *testing.T) { + output.ResetAgentMode() + t.Cleanup(output.ResetAgentMode) + + args := docsSearchArguments("personal access token") + for _, field := range []string{"initiator", "purpose", "why", "question"} { + if args[field] == "" { + t.Errorf("argument %q is empty; the search tool requires it", field) + } + } + if args["question"] != "personal access token" || args["initiator"] != "human" { + t.Errorf("arguments = %v, want the query as question and initiator human", args) + } + + t.Setenv("FORCE_AGENT_MODE", "1") + output.InitAgentMode(&cobra.Command{Use: "vf"}) + if got := docsSearchArguments("x")["initiator"]; got != "agent" { + t.Errorf("initiator in agent mode = %q, want agent", got) + } +} diff --git a/internal/flagutil/stringvalue_test.go b/internal/flagutil/stringvalue_test.go new file mode 100644 index 00000000..503d7b90 --- /dev/null +++ b/internal/flagutil/stringvalue_test.go @@ -0,0 +1,47 @@ +package flagutil + +import ( + "reflect" + "testing" + + "github.com/voiceflow/cli/internal/sdk/optionalnullable" +) + +// unionWithText is shaped like a generated union that has a string member, +// such as components.TranscriptFilterValue1. +type unionWithText struct { + Str *string `union:"member"` + Number *float64 `union:"member"` + Type string +} + +// colour is shaped like a generated string enum. +type colour string + +// A JSON flag takes raw text only when its field holds a string. The behaviour +// tests pin this on real flags, but the spec decides which shapes those flags +// have, and a regeneration took away the one the Markup check relied on. No +// flag today is a union with a string member outside a list, so this is the +// only place that case is checked. These types do not change with the spec. +func TestTargetsStringValue(t *testing.T) { + cases := []struct { + name string + typ reflect.Type + want bool + }{ + {"string", reflect.TypeFor[string](), true}, + {"pointer to string", reflect.TypeFor[*string](), true}, + {"string enum", reflect.TypeFor[colour](), true}, + {"optional nullable string", reflect.TypeFor[optionalnullable.OptionalNullable[string]](), true}, + {"union with a string member", reflect.TypeFor[unionWithText](), false}, + {"list of unions", reflect.TypeFor[[]unionWithText](), false}, + {"list of strings", reflect.TypeFor[[]string](), false}, + {"map of strings", reflect.TypeFor[map[string]string](), false}, + {"optional nullable union", reflect.TypeFor[optionalnullable.OptionalNullable[unionWithText]](), false}, + } + for _, tc := range cases { + if got := targetsStringValue(tc.typ); got != tc.want { + t.Errorf("targetsStringValue(%s) = %v, want %v", tc.name, got, tc.want) + } + } +} diff --git a/test/env.ts b/test/env.ts new file mode 100644 index 00000000..1f1bc6fb --- /dev/null +++ b/test/env.ts @@ -0,0 +1,24 @@ +import dotenv from 'dotenv'; + +// The one VF_ variable a run may set from outside .env.test: it is how a run +// asks to skip the integration tests, and CI sets it in its environment. +const SKIP_INTEGRATION_TESTS = 'VF_SKIP_INTEGRATION_TESTS'; + +/** + * Loads the suite's VF_* settings from .env.test, and from nowhere else. + * + * Developers often export VF_TOKEN, VF_WORKSPACE_ID or VF_OUTPUT_FORMAT in + * their shell for everyday use, and dotenv never overrides a variable that is + * already set, so those values used to win. With a real token and workspace + * in the shell, the integration tests would create and delete projects in that + * workspace, and every spawned vf would see the token. So every other VF_ + * variable is dropped first. + */ +export function loadTestEnv(): void { + for (const name of Object.keys(process.env)) { + if (name.startsWith('VF_') && name !== SKIP_INTEGRATION_TESTS) { + delete process.env[name]; + } + } + dotenv.config({ path: '.env.test' }); +} diff --git a/test/flag-errors.test.ts b/test/flag-errors.test.ts index 3e0b3c5a..7c745d68 100644 --- a/test/flag-errors.test.ts +++ b/test/flag-errors.test.ts @@ -16,7 +16,7 @@ import { execa } from 'execa'; import * as path from 'node:path'; -import { describe, expect, it } from 'vitest'; +import { beforeAll, describe, expect, it } from 'vitest'; const VF = path.resolve(__dirname, '..', 'vf'); @@ -119,29 +119,127 @@ describe('structured flags still reject raw text', () => { }); }); +/** Collapses whitespace runs, since help indents a description's later lines. */ +const squash = (text: string) => text.replace(/\s+/g, ' '); + +/** The escapes a KDL string can hold, beyond \u{...}. */ +const KDL_ESCAPES: Record = { n: '\n', r: '\r', t: '\t', b: '\b', f: '\f', s: ' ', '"': '"', '\\': '\\', '/': '/' }; + +/** Decodes a KDL string body, including \u{...}, which JSON.parse rejects. */ +function unescapeKDL(raw: string): string { + return raw.replace(/\\(?:u\{([0-9a-fA-F]{1,6})\}|(.))/g, (escape, hex?: string, char?: string) => + hex !== undefined ? String.fromCodePoint(parseInt(hex, 16)) : (KDL_ESCAPES[char ?? ''] ?? escape)); +} + +/** Matches the start of any flag's entry in --help. */ +const FLAG_ENTRY = /^\s+(?:-\w, )?--[\w-]/; + +/** + * One flag's own entry in --help: its line, and the lines its description + * wraps onto, up to the next flag or the end of the section. + */ +function helpEntry(help: string, name: string): string | undefined { + const lines = help.split('\n'); + const start = lines.findIndex((line) => new RegExp(`^\\s+(?:-\\w, )?--${name}(?=\\s|$)`).test(line)); + if (start < 0) return undefined; + let end = start + 1; + while (end < lines.length && /^\s+\S/.test(lines[end]!) && !FLAG_ENTRY.test(lines[end]!)) end += 1; + return lines.slice(start, end).join('\n'); +} + +/** + * The labels pflag gives a flag's value: its type, renamed for a few kinds. + * A boolean has none. A label outside this list is a word lifted from the + * description. + */ +const TYPE_LABELS = new Set(['string', 'stringArray', 'strings', 'int', 'ints', 'uint', 'uints', 'float', 'duration', 'bools']); + +/** A flag whose description, as the spec wrote it, uses backticks. */ +interface BacktickedFlag { + command: string[]; + name: string; + description: string; +} + +/** + * Every flag whose description uses backticks, read from `vf --usage`: the KDL + * schema keeps each description exactly as the spec wrote it, where --help + * shows it after the CLI has rewritten it. + */ +async function backtickedFlags(): Promise { + const { stdout } = await run(['--usage']); + const flags: BacktickedFlag[] = []; + const blocks: Array = []; // one per open { }, null when it is not a command + for (const line of stdout.split('\n')) { + if (/^\s*\}\s*$/.test(line)) { + blocks.pop(); + continue; + } + const flag = line.match(/^\s*flag "[^"]*--([\w-]+)[^"]*" help="((?:[^"\\]|\\.)*)"/); + const [, name, help] = flag ?? []; + if (name && help?.includes('`')) { + flags.push({ + command: blocks.filter((block): block is string => block !== null), + name, + description: unescapeKDL(help), + }); + } + if (/\{\s*$/.test(line)) blocks.push(line.match(/^\s*cmd "([^"]+)"/)?.[1] ?? null); + } + return flags; +} + describe('help shows the flag type, not a word from its description', () => { // pflag reads the first back-quoted word in a usage string as the value - // placeholder. Descriptions come from the OpenAPI spec, where backticks are - // prose emphasis, so --instructions used to render as `--instructions Name`. - const cases: Array<[cmd: string[], flag: string, wrong: string]> = [ - [['agent', 'update'], 'instructions', 'Name'], - [['agent', 'update'], 'prompt', 'Name'], - [['transcript', 'search'], 'version-param', 'environmentAlias'], - ]; + // placeholder, and strips that pair of backticks from the prose. Descriptions + // come from the OpenAPI spec, where backticks are emphasis, so flags rendered + // as `--version-param environmentAlias` with the quotes gone from the sentence. + // + // The flags come from `vf --usage` rather than a hand-picked list. The list + // went stale on a regeneration: the spec dropped the backticks from its + // flags' descriptions, and its checks kept passing on flags that no longer had + // anything to check. + let flags: BacktickedFlag[] = []; - for (const [cmd, flag, wrong] of cases) { - it(`${cmd.join(' ')} --${flag} is labelled string, not ${wrong}`, async () => { - const r = await run([...cmd, '--help']); - const line = (r.stdout + r.stderr).split('\n').find((l) => l.includes(`--${flag} `)); - expect(line, `no help line for --${flag}`).toBeDefined(); - expect(line).toContain(`--${flag} string`); - expect(line).not.toContain(`--${flag} ${wrong}`); - }); - } + beforeAll(async () => { + flags = await backtickedFlags(); + }); + + it('finds flags whose descriptions use backticks', () => { + // Zero would leave the next case passing without checking anything. If the + // spec really has stopped using backticks, this block can go. + expect(flags.length).toBeGreaterThan(0); + }); + + it('labels every one of them by its type and keeps its backticks as quotes', async () => { + // Each flag is checked against its own entry in --help, so a flag cannot + // pass on another flag's text. Had pflag lifted a word, the label would be + // that word, and the word would appear bare in the sentence. + const commands = [...new Set(flags.map((flag) => flag.command.join(' ')))]; + const helps = new Map(await Promise.all(commands.map(async (command) => { + const { stdout, stderr } = await run([...command.split(' ').filter(Boolean), '--help']); + return [command, stdout + stderr] as const; + }))); - it('keeps the description prose readable after backticks are neutralized', async () => { - const r = await run(['agent', 'update', '--help']); - expect(r.stdout + r.stderr).toContain("Backticked 'Name' resolves to"); + const problems: string[] = []; + for (const { command, name, description } of flags) { + const entry = helpEntry(helps.get(command.join(' ')) ?? '', name); + const where = `vf ${command.join(' ')} --${name}`; + if (entry === undefined) { + problems.push(`${where}: no help entry`); + continue; + } + // The label runs to the gap before the description; a lifted one can hold + // spaces, as in "{ key, values }". + const label = entry.match(new RegExp(`--${name}(?: (\\S.*?))?(?:\\s{2,}|$)`, 'm'))?.[1]; + if (label !== undefined && !TYPE_LABELS.has(label)) { + problems.push(`${where}: labelled "${label}"`); + } + if (!squash(entry).includes(squash(description.replaceAll('`', "'")))) { + problems.push(`${where}: description changed: ${squash(entry).trim()}`); + } + } + expect(problems).toEqual([]); }); }); diff --git a/test/flag-raw-text.test.ts b/test/flag-raw-text.test.ts index e5c4e5c9..a888f325 100644 --- a/test/flag-raw-text.test.ts +++ b/test/flag-raw-text.test.ts @@ -2,8 +2,9 @@ // string accept prose without JSON quoting. // // The two assertions this change lives or dies on are at the bottom: the wire -// payload is unchanged, and Markup fields are untouched. Both were raised in -// review and both are pinned here rather than argued. +// payload is unchanged, and union fields (Markup, when this was written) are +// untouched. Both were raised in review and both are pinned here rather than +// argued. // // Requires: go build -o vf ./cmd/vf @@ -78,14 +79,27 @@ describe('the fallback changes encoding, not types', () => { expect(a, 'the two input forms disagree on the wire').toBe(sent(quoted.stderr + quoted.stdout, 'instructions')); }); - // --url on mcp-server create is []components.Markup. Markup is a union struct, - // so stringLikeJSONTarget rejects it and the fallback never runs. If this ever - // starts passing, the change has grown past what it was reviewed as. - it('leaves Markup-valued flags strict', async () => { + // --filters on transcript search is []components.TranscriptFilterUnion, a list + // of unions: the shape --url on mcp-server create had as []components.Markup + // until a regeneration made that field a plain string. A list is not a + // string, so the fallback never runs and raw text is still an error. + // + // Both ways of failing start "invalid value for --filters", so the check is + // on the rest. Strict, the text is "not valid JSON". Had the fallback run, the + // text would have been quoted into valid JSON that then failed to decode as a + // list, and the error would say so. + // + // The sharper case, a union with a string member that is not in a list, would + // decode quoted text if the fallback ever ran on it. No flag has that shape + // today, so internal/flagutil/stringvalue_test.go pins it on types the spec + // cannot change. + it('leaves union-valued flags strict', async () => { const r = await run([ - 'mcp-server', 'create', '--project-id', 'p', '--environment-alias', 'main', - '--dry-run', '--token', 'vfp_x', '--name', 'n', '--url', 'not json', + 'transcript', 'search', '--project-id', 'p', '--environment-alias', 'main', + '--dry-run', '--token', 'vfp_x', '--filters', 'not json', ]); - expect(r.stderr, 'a Markup field accepted raw text').toContain('invalid value for --url'); + expect(r.exitCode).not.toBe(0); + expect(r.stderr, 'the raw text was not refused as JSON').toContain('the value is not valid JSON'); + expect(r.stderr, 'the raw-text fallback ran on a union field').not.toContain('valid JSON but not the shape'); }); }); diff --git a/test/isolation.test.ts b/test/isolation.test.ts new file mode 100644 index 00000000..82f1df4d --- /dev/null +++ b/test/isolation.test.ts @@ -0,0 +1,32 @@ +// The suite must never reach the developer's own credentials. test/setup.ts +// gives every spawned vf an empty HOME and no session bus, and test/env.ts +// keeps the shell's VF_* variables out. Both rely on how today's platforms +// find their stores: the macOS Keychain through $HOME, a Linux keyring over +// D-Bus. This checks the result from outside, the way every test runs vf, so +// that if a platform ever finds its store some other way, the suite fails here +// instead of quietly running against a real account. +// +// Requires: go build -o vf ./cmd/vf + +import { execa } from 'execa'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const VF = path.resolve(__dirname, '..', 'vf'); + +describe('a vf spawned by this suite', () => { + it('finds no token and no OAuth session, and no config in the real home', async () => { + const result = await execa({ reject: false, stdin: 'ignore' })(VF, ['whoami']); + + expect(result.exitCode, result.stderr).toBe(0); + // "[unset]" means no flag, VF_TOKEN, OS keyring entry or config file. + expect(result.stdout).toMatch(/--token\s+\[unset\s*\]/); + expect(result.stdout).toMatch(/OAuth session:\s+not signed in/); + + // os.userInfo() reads the user database, not $HOME, so it names the real home. + const configFile = result.stdout.match(/^Config file: (.+)$/m)?.[1] ?? ''; + expect(configFile).not.toBe(''); + expect(configFile.startsWith(os.userInfo().homedir + path.sep)).toBe(false); + }); +}); diff --git a/test/setup.ts b/test/setup.ts index c226d8f7..2aff29fb 100644 --- a/test/setup.ts +++ b/test/setup.ts @@ -1,2 +1,33 @@ -import dotenv from 'dotenv'; -dotenv.config({ path: '.env.test' }); +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterAll } from 'vitest'; + +import { loadTestEnv } from './env'; + +// VF_* settings come from .env.test alone, never from the developer's shell. +loadTestEnv(); + +// Every vf this suite spawns inherits this process's environment, and vf keeps +// credentials in two places: ~/.config/vf (config.yaml and the OAuth session) +// and the OS keyring. Run against the developer's own, the suite was neither +// hermetic nor harmless: tests that expect no credentials found a session and +// failed, and a run refreshed the real OAuth session, rewriting oauth.json and +// rotating the tokens in the keyring. +// +// So each test file gets an empty HOME of its own, and no way to a keyring: +// - macOS: /usr/bin/security finds the login Keychain through $HOME, so under +// an empty HOME there is no default keychain to read or write. +// - Linux: the Secret Service is reached over the D-Bus session bus, found +// through DBUS_SESSION_BUS_ADDRESS or $XDG_RUNTIME_DIR/bus, never $HOME. +// An address whose socket does not exist wins over the fallback and fails +// at once, and vf then treats the keyring as unavailable. +// The suite runs only on Unix, since it spawns ./vf and sh. The integration +// tests are unaffected: they authenticate with VF_TOKEN from .env.test. +const home = fs.mkdtempSync(path.join(os.tmpdir(), 'vf-test-home-')); +process.env.HOME = home; +process.env.DBUS_SESSION_BUS_ADDRESS = `unix:path=${path.join(home, 'no-session-bus')}`; + +afterAll(() => { + fs.rmSync(home, { recursive: true, force: true }); +}); diff --git a/vitest.config.mts b/vitest.config.mts index c529167a..806a8bde 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -1,7 +1,8 @@ import baseConfig from '@voiceflow/vitest-config/unit'; -import dotenv from 'dotenv'; import { mergeConfig, type ViteUserConfig } from 'vitest/config'; +import { loadTestEnv } from './test/env'; + // Two kinds of test live in test/: // // test/vf-*.test.ts drive the real API — they create and delete projects @@ -19,7 +20,9 @@ import { mergeConfig, type ViteUserConfig } from 'vitest/config'; // // Credentials are resolved here rather than in test/setup.ts because setupFiles // run after the config is built, which is too late to choose what to include. -dotenv.config({ path: '.env.test' }); +// They come from .env.test alone, never from the developer's shell: see +// test/env.ts. +loadTestEnv(); const hasCredentials = Boolean(process.env.VF_TOKEN && process.env.VF_WORKSPACE_ID); const integrationOptOut = process.env.VF_SKIP_INTEGRATION_TESTS === '1';