From be842d46794b0fed1ab0968b8f13f07c72cff19e Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 15:05:04 +0300 Subject: [PATCH 01/18] fix(output): never emit a zero timestamp as data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The API omits created_at on /v1/ssh-keys and sends fingerprint: null. Marshaling the SDK struct turned those into 0001-01-01T00:00:00Z and "", so an age-based reaper ("older than 2h => delete") read every key as ancient and deleted keys belonging to running jobs. Add CLI-owned view types that map a zero time.Time to nil so omitempty drops the field, and render absent values as "-" in table output. One shared type backs both the CLI and the MCP tools, so the two JSON contracts cannot drift. Absent is safe; wrong is dangerous. This hides the damage — the API gap itself still needs a backend ticket. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/mcp/tools_ssh.go | 6 +- internal/verda-cli/cmd/mcp/tools_ssh_test.go | 131 +++++++++++++ internal/verda-cli/cmd/sshkey/CLAUDE.md | 14 ++ internal/verda-cli/cmd/sshkey/list.go | 13 +- internal/verda-cli/cmd/sshkey/list_test.go | 172 +++++++++++++++++ .../verda-cli/cmd/startupscript/CLAUDE.md | 10 + internal/verda-cli/cmd/startupscript/list.go | 14 +- .../verda-cli/cmd/startupscript/list_test.go | 144 ++++++++++++++ internal/verda-cli/cmd/util/views.go | 116 ++++++++++++ internal/verda-cli/cmd/util/views_test.go | 179 ++++++++++++++++++ 10 files changed, 787 insertions(+), 12 deletions(-) create mode 100644 internal/verda-cli/cmd/mcp/tools_ssh_test.go create mode 100644 internal/verda-cli/cmd/sshkey/list_test.go create mode 100644 internal/verda-cli/cmd/startupscript/list_test.go create mode 100644 internal/verda-cli/cmd/util/views.go create mode 100644 internal/verda-cli/cmd/util/views_test.go diff --git a/internal/verda-cli/cmd/mcp/tools_ssh.go b/internal/verda-cli/cmd/mcp/tools_ssh.go index 5668beb..c2f1889 100644 --- a/internal/verda-cli/cmd/mcp/tools_ssh.go +++ b/internal/verda-cli/cmd/mcp/tools_ssh.go @@ -21,6 +21,8 @@ import ( "github.com/mark3labs/mcp-go/mcp" "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" ) func (s *Server) registerSSHTools() { @@ -80,7 +82,7 @@ func (s *Server) handleListSSHKeys(ctx context.Context, req mcp.CallToolRequest) keys = filtered } - return jsonResult(keys) + return jsonResult(cmdutil.NewSSHKeyViews(keys)) } //nolint:gocritic // hugeParam: handler signature defined by mcp-go. @@ -106,7 +108,7 @@ func (s *Server) handleAddSSHKey(ctx context.Context, req mcp.CallToolRequest) ( if err != nil { return mcp.NewToolResultError(err.Error()), nil } - return jsonResult(key) + return jsonResult(cmdutil.NewSSHKeyView(key)) } //nolint:gocritic // hugeParam: handler signature defined by mcp-go. diff --git a/internal/verda-cli/cmd/mcp/tools_ssh_test.go b/internal/verda-cli/cmd/mcp/tools_ssh_test.go new file mode 100644 index 0000000..521cb7b --- /dev/null +++ b/internal/verda-cli/cmd/mcp/tools_ssh_test.go @@ -0,0 +1,131 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package mcp + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" +) + +// newSSHKeyServer wires an MCP Server to a stub replaying an exact +// /ssh-keys body. The SDK's testutil.MockServer cannot stand in: its +// handleGetSSHKeys hardcodes CreatedAt: time.Now(), which is precisely the +// field whose absence is under test. +func newSSHKeyServer(t *testing.T, body string) *Server { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "access_token": "test-token", + "token_type": "Bearer", + }) + }) + mux.HandleFunc("GET /ssh-keys", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + client, err := verda.NewClient( + verda.WithBaseURL(srv.URL), + verda.WithClientID("test-id"), + verda.WithClientSecret("test-secret"), + ) + if err != nil { + t.Fatalf("creating client: %v", err) + } + return NewServer(client) +} + +// The MCP surface is where an autonomous reaper actually reads created_at, so +// a zero timestamp here is the one that deletes live keys. Verbatim staging +// body: no created_at key, fingerprint null. +func TestListSSHKeysOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + body := `[{"id":"4d13391d-bdef-49ec-84de-53a2f6174905","name":"meng",` + + `"key":"ssh-ed25519 AAAA","fingerprint":null}]` + s := newSSHKeyServer(t, body) + + res, err := s.handleListSSHKeys(context.Background(), callReq("list_ssh_keys", nil)) + if err != nil { + t.Fatalf("handleListSSHKeys: %v", err) + } + got := resultText(t, res) + + if strings.Contains(got, "0001-01-01") { + t.Errorf("MCP result emits a zero timestamp as data:\n%s", got) + } + if strings.Contains(got, "created_at") { + t.Errorf("created_at present though the API never sent it:\n%s", got) + } + if !strings.Contains(got, "4d13391d-bdef-49ec-84de-53a2f6174905") { + t.Errorf("key id missing:\n%s", got) + } +} + +func TestListSSHKeysKeepsRealCreatedAt(t *testing.T) { + t.Parallel() + + body := `[{"id":"k-1","name":"real","key":"ssh-ed25519 AAA","fingerprint":"SHA256:abc",` + + `"created_at":"2026-08-11T18:51:12.577Z"}]` + s := newSSHKeyServer(t, body) + + res, err := s.handleListSSHKeys(context.Background(), callReq("list_ssh_keys", nil)) + if err != nil { + t.Fatalf("handleListSSHKeys: %v", err) + } + got := resultText(t, res) + + if !strings.Contains(got, "2026-08-11T18:51:12") { + t.Errorf("real created_at was dropped:\n%s", got) + } + if !strings.Contains(got, "SHA256:abc") { + t.Errorf("fingerprint was dropped:\n%s", got) + } +} + +// The search filter must keep operating on the SDK values before conversion. +func TestListSSHKeysSearchStillFilters(t *testing.T) { + t.Parallel() + + body := `[{"id":"k-1","name":"alice","key":"A"},{"id":"k-2","name":"bob","key":"B"}]` + s := newSSHKeyServer(t, body) + + res, err := s.handleListSSHKeys(context.Background(), + callReq("list_ssh_keys", map[string]any{"search": "bob"})) + if err != nil { + t.Fatalf("handleListSSHKeys: %v", err) + } + got := resultText(t, res) + + if !strings.Contains(got, "bob") { + t.Errorf("search dropped the matching key:\n%s", got) + } + if strings.Contains(got, "alice") { + t.Errorf("search kept a non-matching key:\n%s", got) + } + if strings.Contains(got, "0001-01-01") { + t.Errorf("filtered result still emits a zero timestamp:\n%s", got) + } +} diff --git a/internal/verda-cli/cmd/sshkey/CLAUDE.md b/internal/verda-cli/cmd/sshkey/CLAUDE.md index 44e385f..69436fa 100644 --- a/internal/verda-cli/cmd/sshkey/CLAUDE.md +++ b/internal/verda-cli/cmd/sshkey/CLAUDE.md @@ -11,11 +11,25 @@ ## Domain-Specific Logic - `list` displays columns: NAME (20-char), ID (36-char UUID), FINGERPRINT +- `list` marshals `cmdutil.SSHKeyView`, never the raw SDK struct. `/v1/ssh-keys` + omits `created_at` and sends `fingerprint: null`; marshaling the SDK type turns + those into `0001-01-01T00:00:00Z` and `""`. A zero timestamp is not harmless — + an age-based reaper (`older than 2h ⇒ delete`) reads it as ancient and deletes + keys belonging to running jobs. Absent is safe, wrong is dangerous. +- The same view type backs the MCP `list_ssh_keys` / `add_ssh_key` tools, so the + CLI and agent JSON contracts cannot drift. - `add` uses `verda.CreateSSHKeyRequest{Name, PublicKey}` and calls `client.SSHKeys.AddSSHKey()` - `delete` interactive mode fetches all keys via `GetAllSSHKeys()` to build the selection list, appends a "Cancel" entry at the end - Deletion always requires confirmation via `prompter.Confirm()`, even when `--id` is provided directly ## Gotchas & Edge Cases +- Table output renders an absent fingerprint as `-` (`cmdutil.TextColumn`), never + a blank column the eye slides over. +- View tags carry both `json:` and `yaml:` — the encoder is `go.yaml.in/yaml/v3`, + which ignores json tags and would emit `createdat`. +- Backend gap, still open: `/v1/ssh-keys` does not return `created_at`. The CLI + fix hides the damage; it does not supply the data. Remove this note when the + API populates the field. - In `add`, if the prompter returns an error (e.g., user presses Ctrl+C), `runAdd` returns `nil` (not the error) -- this is intentional to avoid printing error messages on user cancellation - Same cancellation pattern in `delete` -- prompter errors return `nil` - `delete` interactive mode uses two separate timeout contexts: one for listing keys, another for the delete call diff --git a/internal/verda-cli/cmd/sshkey/list.go b/internal/verda-cli/cmd/sshkey/list.go index 6a93f13..773c994 100644 --- a/internal/verda-cli/cmd/sshkey/list.go +++ b/internal/verda-cli/cmd/sshkey/list.go @@ -67,21 +67,24 @@ func runList(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams) cmdutil.DebugJSON(ioStreams.ErrOut, f.Debug(), fmt.Sprintf("API response: %d SSH key(s):", len(keys)), keys) + views := cmdutil.NewSSHKeyViews(keys) + // Structured output: emit JSON/YAML and return. - if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), keys); wrote { + if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), views); wrote { return err } - if len(keys) == 0 { + if len(views) == 0 { _, _ = fmt.Fprintln(ioStreams.Out, "No SSH keys found.") return nil } - _, _ = fmt.Fprintf(ioStreams.Out, " %d SSH key(s) found\n\n", len(keys)) + _, _ = fmt.Fprintf(ioStreams.Out, " %d SSH key(s) found\n\n", len(views)) _, _ = fmt.Fprintf(ioStreams.Out, " %-20s %-36s %s\n", "NAME", "ID", "FINGERPRINT") _, _ = fmt.Fprintf(ioStreams.Out, " %-20s %-36s %s\n", "----", "--", "-----------") - for _, k := range keys { - _, _ = fmt.Fprintf(ioStreams.Out, " %-20s %-36s %s\n", k.Name, k.ID, k.Fingerprint) + for i := range views { + v := &views[i] + _, _ = fmt.Fprintf(ioStreams.Out, " %-20s %-36s %s\n", v.Name, v.ID, cmdutil.TextColumn(v.Fingerprint)) } return nil } diff --git a/internal/verda-cli/cmd/sshkey/list_test.go b/internal/verda-cli/cmd/sshkey/list_test.go new file mode 100644 index 0000000..f2ab1ac --- /dev/null +++ b/internal/verda-cli/cmd/sshkey/list_test.go @@ -0,0 +1,172 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sshkey + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" +) + +// stagingBodyNoCreatedAt is the verbatim /v1/ssh-keys response captured from +// staging on 2026-08-12: no created_at key at all, fingerprint null. The SDK's +// testutil.MockServer cannot stand in here — it hardcodes CreatedAt: time.Now(). +const stagingBodyNoCreatedAt = `[{"id":"4d13391d-bdef-49ec-84de-53a2f6174905",` + + `"name":"meng",` + + `"key":"ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIMtJUhkgcr0KR5OYrdQAoY/um6pNQ4RwlUK07tE4kUgq meng@datacrunch.io",` + + `"fingerprint":null}]` + +func newSSHKeyTestClient(t *testing.T, body string) *verda.Client { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "access_token": "test-token", + "token_type": "Bearer", + }) + }) + mux.HandleFunc("GET /ssh-keys", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + client, err := verda.NewClient( + verda.WithBaseURL(srv.URL), + verda.WithClientID("test-id"), + verda.WithClientSecret("test-secret"), + ) + if err != nil { + t.Fatalf("creating client: %v", err) + } + return client +} + +func runListCmd(t *testing.T, body, format string) string { + t.Helper() + var out bytes.Buffer + ioStreams := cmdutil.IOStreams{Out: &out, ErrOut: &bytes.Buffer{}} + f := &cmdutil.TestFactory{ + ClientOverride: newSSHKeyTestClient(t, body), + OutputFormatOverride: format, + } + + root := &cobra.Command{Use: "verda", SilenceUsage: true, SilenceErrors: true} + root.AddCommand(NewCmdSSHKey(f, ioStreams)) + root.SetArgs([]string{"ssh-key", "list"}) + + if err := root.Execute(); err != nil { + t.Fatalf("ssh-key list: %v", err) + } + return out.String() +} + +// A key the API sent no created_at for must not gain one. Emitting Go's zero +// time makes an age-based reaper ("older than 2h ⇒ delete") treat every key as +// ancient and delete keys belonging to running jobs. +func TestListOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + got := runListCmd(t, stagingBodyNoCreatedAt, "json") + + if strings.Contains(got, "0001-01-01") { + t.Errorf("output emits a zero timestamp as data:\n%s", got) + } + if strings.Contains(got, "created_at") { + t.Errorf("created_at present though the API never sent it:\n%s", got) + } + if !strings.Contains(got, "4d13391d-bdef-49ec-84de-53a2f6174905") { + t.Errorf("key id missing from output:\n%s", got) + } +} + +// A real timestamp must survive untouched — the fix omits absent values, it +// does not drop the field wholesale. +func TestListKeepsRealCreatedAt(t *testing.T) { + t.Parallel() + + body := `[{"id":"k-1","name":"real","key":"ssh-ed25519 AAA","fingerprint":"SHA256:abc",` + + `"created_at":"2026-08-11T18:51:12.577Z"}]` + got := runListCmd(t, body, "json") + + if !strings.Contains(got, "2026-08-11T18:51:12") { + t.Errorf("real created_at was dropped:\n%s", got) + } + if !strings.Contains(got, "SHA256:abc") { + t.Errorf("fingerprint was dropped:\n%s", got) + } +} + +// One row without a timestamp must not suppress another row's. +func TestListMixedCreatedAt(t *testing.T) { + t.Parallel() + + body := `[{"id":"k-1","name":"nostamp","key":"ssh-ed25519 AAA","fingerprint":null},` + + `{"id":"k-2","name":"stamped","key":"ssh-ed25519 BBB","fingerprint":"SHA256:xyz",` + + `"created_at":"2026-08-11T18:51:12.577Z"}]` + got := runListCmd(t, body, "json") + + if strings.Contains(got, "0001-01-01") { + t.Errorf("zero timestamp leaked for the row without one:\n%s", got) + } + if !strings.Contains(got, "2026-08-11T18:51:12") { + t.Errorf("the stamped row lost its created_at:\n%s", got) + } + if strings.Count(got, "created_at") != 1 { + t.Errorf("created_at count = %d, want exactly 1:\n%s", strings.Count(got, "created_at"), got) + } +} + +func TestListEmpty(t *testing.T) { + t.Parallel() + + got := runListCmd(t, `[]`, "json") + if strings.Contains(got, "0001-01-01") { + t.Errorf("empty list emitted a timestamp:\n%s", got) + } +} + +// Table output must not print a blank fingerprint column. Asserted on the data +// row only — the header's "----" separator would satisfy a naive dash check. +func TestListTableShowsDashForAbsentFingerprint(t *testing.T) { + t.Parallel() + + got := runListCmd(t, stagingBodyNoCreatedAt, "table") + + var dataRow string + for line := range strings.SplitSeq(got, "\n") { + if strings.Contains(line, "4d13391d-bdef-49ec-84de-53a2f6174905") { + dataRow = line + break + } + } + if dataRow == "" { + t.Fatalf("no data row for the key in table output:\n%s", got) + } + if strings.TrimRight(dataRow, " ") != strings.TrimRight(dataRow, " -") { + return // ends in "-": absent fingerprint rendered explicitly + } + t.Errorf("absent fingerprint rendered blank; want %q at end of row %q", "-", dataRow) +} diff --git a/internal/verda-cli/cmd/startupscript/CLAUDE.md b/internal/verda-cli/cmd/startupscript/CLAUDE.md index 7e5831a..2de05c0 100644 --- a/internal/verda-cli/cmd/startupscript/CLAUDE.md +++ b/internal/verda-cli/cmd/startupscript/CLAUDE.md @@ -11,6 +11,11 @@ ## Domain-Specific Logic - `list` displays columns: NAME (20-char), ID (36-char UUID), CREATED (formatted `2006-01-02 15:04`) +- `list` marshals `cmdutil.StartupScriptView`, never the raw SDK struct. When the + API omits `created_at`, the SDK's `time.Time` zero value renders as + `0001-01-01T00:00:00Z` in JSON and `0001-01-01 00:00` in the table — a + plausible-looking date that an age-based reaper acts on. Absent stays absent; + the table prints `-` via `cmdutil.TimeColumn`. - `add` has three input modes resolved in this priority: `--file` > `--script` > interactive prompt - Interactive add offers two source options: "Load from file" (text input for path) or "Paste content" (TUI editor with `WithEditorDefault("#!/bin/bash\n\n# Your startup script here\n")` and `WithFileExt(".sh")`) - `add` validates that final content is non-empty after trimming whitespace @@ -24,6 +29,11 @@ - `add` imports `github.com/verda-cloud/verda-cli/pkg/tui` for `tui.WithEditorDefault` and `tui.WithFileExt` editor options - `delete` interactive mode uses two separate timeout contexts: one for listing, another for deleting - When no scripts exist, both `list` and `delete` print a friendly message and return `nil` +- View tags carry both `json:` and `yaml:` — the encoder is `go.yaml.in/yaml/v3`, + which ignores json tags and would emit `createdat` +- Whether `/v1/scripts` actually omits `created_at` is **unconfirmed** (no scripts + on the test account); the view is correct either way. Confirmed for + `/v1/ssh-keys` — see `cmd/sshkey/CLAUDE.md` ## Relationships - Depends on `cmdutil.Factory` for VerdaClient, Prompter, Status, Debug, Options diff --git a/internal/verda-cli/cmd/startupscript/list.go b/internal/verda-cli/cmd/startupscript/list.go index a60be4b..d4bad5c 100644 --- a/internal/verda-cli/cmd/startupscript/list.go +++ b/internal/verda-cli/cmd/startupscript/list.go @@ -67,21 +67,25 @@ func runList(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams) cmdutil.DebugJSON(ioStreams.ErrOut, f.Debug(), fmt.Sprintf("API response: %d startup script(s):", len(scripts)), scripts) + views := cmdutil.NewStartupScriptViews(scripts) + // Structured output: emit JSON/YAML and return. - if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), scripts); wrote { + if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), views); wrote { return err } - if len(scripts) == 0 { + if len(views) == 0 { _, _ = fmt.Fprintln(ioStreams.Out, "No startup scripts found.") return nil } - _, _ = fmt.Fprintf(ioStreams.Out, " %d startup script(s) found\n\n", len(scripts)) + _, _ = fmt.Fprintf(ioStreams.Out, " %d startup script(s) found\n\n", len(views)) _, _ = fmt.Fprintf(ioStreams.Out, " %-20s %-36s %s\n", "NAME", "ID", "CREATED") _, _ = fmt.Fprintf(ioStreams.Out, " %-20s %-36s %s\n", "----", "--", "-------") - for _, s := range scripts { - _, _ = fmt.Fprintf(ioStreams.Out, " %-20s %-36s %s\n", s.Name, s.ID, s.CreatedAt.Format("2006-01-02 15:04")) + for i := range views { + v := &views[i] + _, _ = fmt.Fprintf(ioStreams.Out, " %-20s %-36s %s\n", + v.Name, v.ID, cmdutil.TimeColumn(v.CreatedAt, "2006-01-02 15:04")) } return nil } diff --git a/internal/verda-cli/cmd/startupscript/list_test.go b/internal/verda-cli/cmd/startupscript/list_test.go new file mode 100644 index 0000000..d5b3888 --- /dev/null +++ b/internal/verda-cli/cmd/startupscript/list_test.go @@ -0,0 +1,144 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package startupscript + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" +) + +func newScriptTestClient(t *testing.T, body string) *verda.Client { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "access_token": "test-token", + "token_type": "Bearer", + }) + }) + mux.HandleFunc("GET /scripts", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + client, err := verda.NewClient( + verda.WithBaseURL(srv.URL), + verda.WithClientID("test-id"), + verda.WithClientSecret("test-secret"), + ) + if err != nil { + t.Fatalf("creating client: %v", err) + } + return client +} + +func runScriptListCmd(t *testing.T, body, format string) string { + t.Helper() + var out bytes.Buffer + ioStreams := cmdutil.IOStreams{Out: &out, ErrOut: &bytes.Buffer{}} + f := &cmdutil.TestFactory{ + ClientOverride: newScriptTestClient(t, body), + OutputFormatOverride: format, + } + + root := &cobra.Command{Use: "verda", SilenceUsage: true, SilenceErrors: true} + root.AddCommand(NewCmdStartupScript(f, ioStreams)) + root.SetArgs([]string{"startup-script", "list"}) + + if err := root.Execute(); err != nil { + t.Fatalf("startup-script list: %v", err) + } + return out.String() +} + +const scriptBodyNoCreatedAt = `[{"id":"s-1","name":"bootstrap","script":"#!/bin/bash\necho hi"}]` + +// Same defect class as ssh-key list: an absent created_at must not be invented. +func TestScriptListOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + got := runScriptListCmd(t, scriptBodyNoCreatedAt, "json") + + if strings.Contains(got, "0001-01-01") { + t.Errorf("output emits a zero timestamp as data:\n%s", got) + } + if strings.Contains(got, "created_at") { + t.Errorf("created_at present though the API never sent it:\n%s", got) + } + if !strings.Contains(got, "bootstrap") { + t.Errorf("script name missing:\n%s", got) + } +} + +// The table formats CreatedAt directly, so the same gap surfaces as +// "0001-01-01 00:00" — a plausible-looking date, which is worse than "-". +func TestScriptListTableShowsDashForAbsentCreatedAt(t *testing.T) { + t.Parallel() + + got := runScriptListCmd(t, scriptBodyNoCreatedAt, "table") + + if strings.Contains(got, "0001-01-01") { + t.Errorf("table emits a zero timestamp:\n%s", got) + } + var dataRow string + for line := range strings.SplitSeq(got, "\n") { + if strings.Contains(line, "bootstrap") { + dataRow = line + break + } + } + if dataRow == "" { + t.Fatalf("no data row in table output:\n%s", got) + } + if !strings.HasSuffix(strings.TrimRight(dataRow, " "), "-") { + t.Errorf("absent CREATED should render as %q, got row %q", "-", dataRow) + } +} + +func TestScriptListKeepsRealCreatedAt(t *testing.T) { + t.Parallel() + + body := `[{"id":"s-1","name":"bootstrap","script":"x","created_at":"2026-08-11T18:51:12.577Z"}]` + + jsonOut := runScriptListCmd(t, body, "json") + if !strings.Contains(jsonOut, "2026-08-11T18:51:12") { + t.Errorf("json lost the timestamp:\n%s", jsonOut) + } + + tableOut := runScriptListCmd(t, body, "table") + if !strings.Contains(tableOut, "2026-08-11 18:51") { + t.Errorf("table lost the timestamp:\n%s", tableOut) + } +} + +func TestScriptListEmpty(t *testing.T) { + t.Parallel() + + if got := runScriptListCmd(t, `[]`, "json"); strings.Contains(got, "0001-01-01") { + t.Errorf("empty list emitted a timestamp:\n%s", got) + } +} diff --git a/internal/verda-cli/cmd/util/views.go b/internal/verda-cli/cmd/util/views.go new file mode 100644 index 0000000..13c9beb --- /dev/null +++ b/internal/verda-cli/cmd/util/views.go @@ -0,0 +1,116 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package util + +import ( + "time" + + "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" +) + +// View types own the JSON/YAML contract for resources whose API payloads have +// optional fields. Marshaling an SDK struct directly turns "the API sent +// nothing" into Go's zero value, and a zero time.Time is not harmless: it +// serializes as 0001-01-01T00:00:00Z, which an age-based reaper +// ("older than 2h ⇒ delete") reads as ancient and acts on. Absent is safe, +// wrong is dangerous — so absent stays absent. +// +// Keys mirror the SDK's own json tags exactly ("key", not "public_key"). yaml +// tags are mandatory: the encoder is go.yaml.in/yaml/v3 (output.go), which +// ignores json tags and would otherwise emit "createdat". + +// nilIfZero maps the zero time to nil so an omitempty tag can drop the field. +// A pointer to a zero value would still marshal. +func nilIfZero(t time.Time) *time.Time { + if t.IsZero() { + return nil + } + return &t +} + +// SSHKeyView is the JSON/YAML shape for one SSH key. +type SSHKeyView struct { + ID string `json:"id" yaml:"id"` + Name string `json:"name" yaml:"name"` + PublicKey string `json:"key" yaml:"key"` + Fingerprint string `json:"fingerprint,omitempty" yaml:"fingerprint,omitempty"` + CreatedAt *time.Time `json:"created_at,omitempty" yaml:"created_at,omitempty"` +} + +// NewSSHKeyView converts one SDK SSH key to its output shape. +func NewSSHKeyView(k *verda.SSHKey) SSHKeyView { + return SSHKeyView{ + ID: k.ID, + Name: k.Name, + PublicKey: k.PublicKey, + Fingerprint: k.Fingerprint, + CreatedAt: nilIfZero(k.CreatedAt), + } +} + +// NewSSHKeyViews converts a slice of SDK SSH keys, preserving order. +func NewSSHKeyViews(keys []verda.SSHKey) []SSHKeyView { + views := make([]SSHKeyView, len(keys)) + for i := range keys { + views[i] = NewSSHKeyView(&keys[i]) + } + return views +} + +// StartupScriptView is the JSON/YAML shape for one startup script. +type StartupScriptView struct { + ID string `json:"id" yaml:"id"` + Name string `json:"name" yaml:"name"` + Script string `json:"script" yaml:"script"` + CreatedAt *time.Time `json:"created_at,omitempty" yaml:"created_at,omitempty"` +} + +// NewStartupScriptView converts one SDK startup script to its output shape. +func NewStartupScriptView(s *verda.StartupScript) StartupScriptView { + return StartupScriptView{ + ID: s.ID, + Name: s.Name, + Script: s.Script, + CreatedAt: nilIfZero(s.CreatedAt), + } +} + +// NewStartupScriptViews converts a slice of SDK startup scripts, preserving order. +func NewStartupScriptViews(scripts []verda.StartupScript) []StartupScriptView { + views := make([]StartupScriptView, len(scripts)) + for i := range scripts { + views[i] = NewStartupScriptView(&scripts[i]) + } + return views +} + +// TimeColumn renders a timestamp for table output. An absent value prints as +// "-" rather than 0001-01-01, so a human reading the table sees "unknown" +// instead of a plausible-looking date. +func TimeColumn(t *time.Time, layout string) string { + if t == nil { + return "-" + } + return t.Format(layout) +} + +// TextColumn renders a possibly-empty string for table output, so an absent +// value is visible as "-" instead of a blank the eye slides over. +func TextColumn(s string) string { + if s == "" { + return "-" + } + return s +} diff --git a/internal/verda-cli/cmd/util/views_test.go b/internal/verda-cli/cmd/util/views_test.go new file mode 100644 index 0000000..8c29d76 --- /dev/null +++ b/internal/verda-cli/cmd/util/views_test.go @@ -0,0 +1,179 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package util + +import ( + "bytes" + "strings" + "testing" + "time" + + "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" +) + +func marshalBoth(t *testing.T, v any) (jsonOut, yamlOut string) { + t.Helper() + var jb, yb bytes.Buffer + if _, err := WriteStructured(&jb, "json", v); err != nil { + t.Fatalf("json: %v", err) + } + if _, err := WriteStructured(&yb, "yaml", v); err != nil { + t.Fatalf("yaml: %v", err) + } + return jb.String(), yb.String() +} + +// A zero CreatedAt must vanish from both encodings — a pointer to a zero value +// would still marshal, so nilIfZero has to return nil, not &zero. +func TestSSHKeyViewOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + view := NewSSHKeyView(&verda.SSHKey{ID: "k-1", Name: "n", PublicKey: "ssh-ed25519 AAA"}) + + if view.CreatedAt != nil { + t.Fatalf("CreatedAt = %v, want nil for a zero time", view.CreatedAt) + } + jsonOut, yamlOut := marshalBoth(t, view) + for name, out := range map[string]string{"json": jsonOut, "yaml": yamlOut} { + if strings.Contains(out, "created_at") { + t.Errorf("%s still carries created_at: %s", name, out) + } + if strings.Contains(out, "0001-01-01") { + t.Errorf("%s emits a zero timestamp: %s", name, out) + } + } +} + +// An empty fingerprint (the API sends null) must be absent, not "". +func TestSSHKeyViewOmitsEmptyFingerprint(t *testing.T) { + t.Parallel() + + jsonOut, yamlOut := marshalBoth(t, NewSSHKeyView(&verda.SSHKey{ID: "k-1", Name: "n"})) + if strings.Contains(jsonOut, "fingerprint") { + t.Errorf("json carries an empty fingerprint: %s", jsonOut) + } + if strings.Contains(yamlOut, "fingerprint") { + t.Errorf("yaml carries an empty fingerprint: %s", yamlOut) + } +} + +// A real timestamp round-trips unchanged, and the yaml tags keep the key +// snake_case — yaml/v3 ignores json tags and would emit "createdat" without them. +func TestSSHKeyViewKeepsRealValues(t *testing.T) { + t.Parallel() + + when := time.Date(2026, 8, 11, 18, 51, 12, 577_000_000, time.UTC) + view := NewSSHKeyView(&verda.SSHKey{ + ID: "k-1", Name: "n", PublicKey: "ssh-ed25519 AAA", + Fingerprint: "SHA256:abc", CreatedAt: when, + }) + + if view.CreatedAt == nil || !view.CreatedAt.Equal(when) { + t.Fatalf("CreatedAt = %v, want %v", view.CreatedAt, when) + } + jsonOut, yamlOut := marshalBoth(t, view) + if !strings.Contains(jsonOut, "2026-08-11T18:51:12") { + t.Errorf("json lost the timestamp: %s", jsonOut) + } + if !strings.Contains(yamlOut, "created_at") { + t.Errorf("yaml key is not snake_case (missing yaml tag?): %s", yamlOut) + } + if strings.Contains(yamlOut, "createdat") { + t.Errorf("yaml fell back to the field name: %s", yamlOut) + } +} + +func TestStartupScriptViewOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + view := NewStartupScriptView(&verda.StartupScript{ID: "s-1", Name: "boot", Script: "#!/bin/sh"}) + + if view.CreatedAt != nil { + t.Fatalf("CreatedAt = %v, want nil", view.CreatedAt) + } + jsonOut, yamlOut := marshalBoth(t, view) + for name, out := range map[string]string{"json": jsonOut, "yaml": yamlOut} { + if strings.Contains(out, "0001-01-01") { + t.Errorf("%s emits a zero timestamp: %s", name, out) + } + } + if !strings.Contains(jsonOut, "#!/bin/sh") { + t.Errorf("script body was dropped: %s", jsonOut) + } +} + +func TestNewViewsPreserveOrderAndLength(t *testing.T) { + t.Parallel() + + keys := []verda.SSHKey{{ID: "a"}, {ID: "b"}, {ID: "c"}} + views := NewSSHKeyViews(keys) + if len(views) != 3 { + t.Fatalf("len = %d, want 3", len(views)) + } + for i, want := range []string{"a", "b", "c"} { + if views[i].ID != want { + t.Errorf("views[%d].ID = %q, want %q", i, views[i].ID, want) + } + } + + if got := NewSSHKeyViews(nil); len(got) != 0 { + t.Errorf("nil input produced %d views", len(got)) + } + if got := NewStartupScriptViews(nil); len(got) != 0 { + t.Errorf("nil input produced %d views", len(got)) + } +} + +// One row's absent timestamp must not affect another's. +func TestSSHKeyViewsMixed(t *testing.T) { + t.Parallel() + + when := time.Date(2026, 8, 11, 18, 51, 12, 0, time.UTC) + views := NewSSHKeyViews([]verda.SSHKey{{ID: "a"}, {ID: "b", CreatedAt: when}}) + + if views[0].CreatedAt != nil { + t.Errorf("row 0 gained a timestamp: %v", views[0].CreatedAt) + } + if views[1].CreatedAt == nil { + t.Fatal("row 1 lost its timestamp") + } + jsonOut, _ := marshalBoth(t, views) + if strings.Count(jsonOut, "created_at") != 1 { + t.Errorf("created_at count = %d, want 1: %s", strings.Count(jsonOut, "created_at"), jsonOut) + } +} + +func TestTimeColumn(t *testing.T) { + t.Parallel() + + if got := TimeColumn(nil, "2006-01-02"); got != "-" { + t.Errorf("nil → %q, want %q", got, "-") + } + when := time.Date(2026, 8, 11, 18, 51, 0, 0, time.UTC) + if got := TimeColumn(&when, "2006-01-02 15:04"); got != "2026-08-11 18:51" { + t.Errorf("got %q", got) + } +} + +func TestTextColumn(t *testing.T) { + t.Parallel() + + if got := TextColumn(""); got != "-" { + t.Errorf("empty → %q, want %q", got, "-") + } + if got := TextColumn("SHA256:abc"); got != "SHA256:abc" { + t.Errorf("got %q", got) + } +} From aebf432e4441c799e52e437a160a5c93be1a8847 Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 15:09:08 +0300 Subject: [PATCH 02/18] fix(s3): honor VERDA_S3_* environment variables Nothing in the tree ever read VERDA_S3_*: the only credential source was the INI file, so CI could not use object storage without writing secrets to disk. API auth already honored VERDA_CLIENT_ID/_SECRET, which made the documented precedence true for API auth and false for S3. Add options.ResolveS3Credentials: load the profile, then overlay VERDA_S3_ACCESS_KEY/_SECRET_KEY/_ENDPOINT/_REGION/_AUTH_MODE per field. Flags still win (NewClient's firstNonEmpty), so the order is flags > env > file. The merge is per field rather than wholesale, so one variable cannot discard a working profile, and an empty variable counts as unset. A missing file or unresolvable path (no HOME in a container) stops being fatal once env alone is complete; HasCredentials() still gates, so a partial set fails loudly. show uses the same resolver -- otherwise it would report "not configured" for an env-only setup that ls handles fine -- and names the applied variables under env_overrides:, never their values. Co-Authored-By: Claude Opus 5 (1M context) --- .../verda-cli/cmd/objectstorage/CLAUDE.md | 24 ++- .../verda-cli/cmd/objectstorage/README.md | 15 ++ .../verda-cli/cmd/objectstorage/configure.go | 11 ++ .../verda-cli/cmd/objectstorage/helper.go | 7 +- .../cmd/objectstorage/helper_test.go | 156 ++++++++++++++++++ internal/verda-cli/cmd/objectstorage/show.go | 7 +- internal/verda-cli/options/s3_credentials.go | 44 +++++ .../verda-cli/options/s3_credentials_test.go | 116 +++++++++++++ 8 files changed, 374 insertions(+), 6 deletions(-) diff --git a/internal/verda-cli/cmd/objectstorage/CLAUDE.md b/internal/verda-cli/cmd/objectstorage/CLAUDE.md index ac49a5e..67cf18a 100644 --- a/internal/verda-cli/cmd/objectstorage/CLAUDE.md +++ b/internal/verda-cli/cmd/objectstorage/CLAUDE.md @@ -19,11 +19,27 @@ ## Domain-Specific Logic ### Credential resolution order -Resolved in `client.go` `NewClient` + `resolveEndpoint`: +Loaded by `options.ResolveS3Credentials` (file + env), then flags applied in +`client.go` `NewClient` + `resolveEndpoint`: 1. Per-invocation flag overrides: `--endpoint`, `--access-key`, `--secret-key`, `--region` -2. `~/.verda/credentials` profile, keys prefixed `verda_s3_` -3. `DefaultEndpoint` fallback for endpoint only (host/region are required via flag or profile) -4. `verda_s3_auth_mode`: `credentials` (implemented), `api` (stub -- not yet implemented) +2. `VERDA_S3_ACCESS_KEY` / `_SECRET_KEY` / `_ENDPOINT` / `_REGION` / `_AUTH_MODE` +3. `~/.verda/credentials` profile, keys prefixed `verda_s3_` +4. `DefaultEndpoint` fallback for endpoint only (host/region are required via flag or profile) +5. `verda_s3_auth_mode`: `credentials` (implemented), `api` (stub -- not yet implemented) + +The env overlay is **per field**, not wholesale: `VERDA_S3_ENDPOINT` alone +overrides the endpoint and keeps the profile's keys. An empty or whitespace-only +variable counts as unset, so `export VERDA_S3_REGION=` cannot blank a good +profile. A complete env set needs no credentials file at all — that is the CI +case, and `loadCredsFromFactory` even tolerates an unresolvable file path (no +`HOME`) when env alone is complete. `HasCredentials()` (access key + secret + +endpoint) still gates, so a partial env fails loudly instead of half-overriding. + +`show` uses the same resolver and prints an `env_overrides:` line naming the +variables in play (**names only, never values**) — without it, `show` would +report "not configured" for an env-only setup that `ls`/`cp` handle fine. +`wizard.go`'s profile picker deliberately does *not* apply the overlay: it +enumerates what is in the file, and an overlay there would invent a profile. ### Profile fallback (s3-specific) S3 commands are in `skipCredentialResolution` (see `cmd/cmd.go`), so `Options.Complete()` never runs and `AuthOptions.Profile` stays empty. `loadCredsFromFactory` in `helper.go` therefore falls back to `defaultProfileName` ("default") when `Profile == ""`. Without this, `LoadS3CredentialsForProfile(path, "")` would load ini.v1's synthetic `DEFAULT` section instead of the user's `[default]` section, and `s3 ls`/`cp`/etc. would falsely report "no S3 credentials configured" right after a successful `s3 configure`. `s3 show` applies the same fallback inline. diff --git a/internal/verda-cli/cmd/objectstorage/README.md b/internal/verda-cli/cmd/objectstorage/README.md index 28a5252..6cfffbb 100644 --- a/internal/verda-cli/cmd/objectstorage/README.md +++ b/internal/verda-cli/cmd/objectstorage/README.md @@ -204,6 +204,21 @@ Per-file progress lines (`uploaded`, `downloaded`, `copied`, `moved`, `deleted`) ## Environment - `VERDA_SHARED_CREDENTIALS_FILE` -- override the default credentials path (`~/.verda/credentials`) +- `VERDA_S3_ACCESS_KEY`, `VERDA_S3_SECRET_KEY`, `VERDA_S3_ENDPOINT`, `VERDA_S3_REGION`, `VERDA_S3_AUTH_MODE` -- S3 credentials from the environment + +Resolution is per field: **flags > environment > credentials-file profile**. +Setting only `VERDA_S3_ENDPOINT` overrides the endpoint and leaves the profile's +keys alone; an empty variable counts as unset. A complete environment set needs +no credentials file at all, so CI never has to write secrets to disk: + +```bash +export VERDA_S3_ACCESS_KEY=... VERDA_S3_SECRET_KEY=... +export VERDA_S3_ENDPOINT=https://objects.fin-03.verda.storage +verda object-storage ls s3://my-bucket +``` + +`verda object-storage show` lists which variables are in effect under +`env_overrides:` (names only -- never values). ## Multiple profiles diff --git a/internal/verda-cli/cmd/objectstorage/configure.go b/internal/verda-cli/cmd/objectstorage/configure.go index e2b8987..f9644cd 100644 --- a/internal/verda-cli/cmd/objectstorage/configure.go +++ b/internal/verda-cli/cmd/objectstorage/configure.go @@ -61,6 +61,17 @@ func NewCmdConfigure(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Comm Default file: ~/.verda/credentials Override with --credentials-file or VERDA_SHARED_CREDENTIALS_FILE. + + Resolution order for every object-storage command, per field: + 1. Command flags (--access-key, --secret-key, --endpoint, --region) + 2. VERDA_S3_ACCESS_KEY, VERDA_S3_SECRET_KEY, VERDA_S3_ENDPOINT, + VERDA_S3_REGION, VERDA_S3_AUTH_MODE + 3. The credentials-file profile above + + The merge is per field, not all-or-nothing: setting only + VERDA_S3_ENDPOINT overrides the endpoint and keeps the profile's keys. + An empty variable counts as unset. A complete env set needs no file at + all, which is how CI avoids writing secrets to disk. `), Example: cmdutil.Examples(` # Interactive wizard (prompts for all fields) diff --git a/internal/verda-cli/cmd/objectstorage/helper.go b/internal/verda-cli/cmd/objectstorage/helper.go index 8584f99..2849661 100644 --- a/internal/verda-cli/cmd/objectstorage/helper.go +++ b/internal/verda-cli/cmd/objectstorage/helper.go @@ -63,9 +63,14 @@ func loadCredsFromFactory(f cmdutil.Factory) (*options.S3Credentials, error) { } path, err := resolveCredentialsFile("") if err != nil { + // No file path to resolve (e.g. no HOME in a container) — VERDA_S3_* may + // still carry a complete set on its own. + if creds, _, envErr := options.ResolveS3Credentials("", profile); envErr == nil { + return creds, nil + } return nil, err } - creds, err := options.LoadS3CredentialsForProfile(path, profile) + creds, _, err := options.ResolveS3Credentials(path, profile) if err != nil { return &options.S3Credentials{}, nil //nolint:nilerr // intentional: missing creds → empty struct so NewClient surfaces the "verda object-storage configure" hint } diff --git a/internal/verda-cli/cmd/objectstorage/helper_test.go b/internal/verda-cli/cmd/objectstorage/helper_test.go index cb725fb..d9dff55 100644 --- a/internal/verda-cli/cmd/objectstorage/helper_test.go +++ b/internal/verda-cli/cmd/objectstorage/helper_test.go @@ -19,6 +19,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" @@ -87,3 +88,158 @@ func TestLoadCredsFromFactoryFallsBackToDefaultProfile(t *testing.T) { t.Errorf("Endpoint = %q, want https://example.invalid", creds.Endpoint) } } + +// s3TestFactory is the shape every S3 command sees in production: S3 commands +// are in skipCredentialResolution, so AuthOptions.Profile is never resolved. +func s3TestFactory() *cmdutil.TestFactory { + return &cmdutil.TestFactory{ + OptionsOverride: &options.Options{ + AuthOptions: &options.AuthOptions{}, + }, + } +} + +func writeS3Profile(t *testing.T, contents string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "credentials") + if err := os.WriteFile(path, []byte(contents), 0o600); err != nil { + t.Fatalf("write creds file: %v", err) + } + return path +} + +// CI has no credentials file and must not have to write secrets to disk: +// VERDA_S3_* alone has to be enough. Before this fix nothing in the tree ever +// read those variables, so a complete env resolved to "no S3 credentials". +func TestLoadCredsFromFactoryHonorsEnvWithoutFile(t *testing.T) { + // No t.Parallel: t.Setenv. + t.Setenv("VERDA_PROFILE", "default") + t.Setenv("VERDA_SHARED_CREDENTIALS_FILE", filepath.Join(t.TempDir(), "absent")) + t.Setenv("VERDA_S3_ACCESS_KEY", "REPLACE_ME_ENV_KEY") + t.Setenv("VERDA_S3_SECRET_KEY", "REPLACE_ME_ENV_SECRET") + t.Setenv("VERDA_S3_ENDPOINT", "https://env.example.invalid") + t.Setenv("VERDA_S3_REGION", "eu-north-1") + + creds, err := loadCredsFromFactory(s3TestFactory()) + if err != nil { + t.Fatalf("loadCredsFromFactory: %v", err) + } + if !creds.HasCredentials() { + t.Fatalf("HasCredentials() = false; env-only credentials were ignored: %+v", redactedCreds(creds)) + } + if creds.AccessKey != "REPLACE_ME_ENV_KEY" || creds.SecretKey != "REPLACE_ME_ENV_SECRET" { + t.Errorf("key material not taken from env: %+v", redactedCreds(creds)) + } + if creds.Endpoint != "https://env.example.invalid" { + t.Errorf("Endpoint = %q, want the env value", creds.Endpoint) + } + if creds.Region != "eu-north-1" { + t.Errorf("Region = %q, want eu-north-1", creds.Region) + } +} + +// The decided semantics: per-field merge, env above file. One env var must not +// discard the rest of a working profile. +func TestLoadCredsFromFactoryEnvOverridesFilePerField(t *testing.T) { + path := writeS3Profile(t, "[default]\n"+ + "verda_s3_access_key = FILE_KEY\n"+ + "verda_s3_secret_key = FILE_SECRET\n"+ + "verda_s3_endpoint = https://file.example.invalid\n"+ + "verda_s3_region = us-east-1\n") + + t.Setenv("VERDA_PROFILE", "default") + t.Setenv("VERDA_SHARED_CREDENTIALS_FILE", path) + t.Setenv("VERDA_S3_ENDPOINT", "https://env.example.invalid") + + creds, err := loadCredsFromFactory(s3TestFactory()) + if err != nil { + t.Fatalf("loadCredsFromFactory: %v", err) + } + if creds.Endpoint != "https://env.example.invalid" { + t.Errorf("Endpoint = %q, want the env override", creds.Endpoint) + } + if creds.AccessKey != "FILE_KEY" || creds.SecretKey != "FILE_SECRET" { + t.Errorf("env endpoint wiped file key material: %+v", redactedCreds(creds)) + } + if creds.Region != "us-east-1" { + t.Errorf("Region = %q, want the file value us-east-1", creds.Region) + } +} + +// An exported-but-empty variable (`export VERDA_S3_REGION=`) means unset, not +// "blank the profile". +func TestLoadCredsFromFactoryEmptyEnvKeepsFileValue(t *testing.T) { + path := writeS3Profile(t, "[default]\n"+ + "verda_s3_access_key = FILE_KEY\n"+ + "verda_s3_secret_key = FILE_SECRET\n"+ + "verda_s3_endpoint = https://file.example.invalid\n"+ + "verda_s3_region = us-east-1\n") + + t.Setenv("VERDA_PROFILE", "default") + t.Setenv("VERDA_SHARED_CREDENTIALS_FILE", path) + t.Setenv("VERDA_S3_REGION", "") + + creds, err := loadCredsFromFactory(s3TestFactory()) + if err != nil { + t.Fatalf("loadCredsFromFactory: %v", err) + } + if creds.Region != "us-east-1" { + t.Errorf("Region = %q; an empty env var blanked the file value", creds.Region) + } +} + +// A partial env set with no file must stay incomplete — NewClient then produces +// the "run configure" hint rather than half-authenticating. +func TestLoadCredsFromFactoryPartialEnvStaysIncomplete(t *testing.T) { + t.Setenv("VERDA_PROFILE", "default") + t.Setenv("VERDA_SHARED_CREDENTIALS_FILE", filepath.Join(t.TempDir(), "absent")) + t.Setenv("VERDA_S3_ACCESS_KEY", "REPLACE_ME_ENV_KEY") + + creds, err := loadCredsFromFactory(s3TestFactory()) + if err != nil { + t.Fatalf("loadCredsFromFactory: %v", err) + } + if creds.HasCredentials() { + t.Errorf("HasCredentials() = true with only an access key set: %+v", redactedCreds(creds)) + } + + _, err = NewClient(context.Background(), creds, creds.AuthMode, ClientOverrides{}) + if err == nil { + t.Fatal("NewClient succeeded on incomplete credentials") + } + if strings.Contains(err.Error(), "REPLACE_ME_ENV_KEY") { + t.Errorf("error message leaks key material: %v", err) + } +} + +// Flags still outrank env — env sits between flags and file. +func TestClientOverridesBeatEnv(t *testing.T) { + t.Setenv("VERDA_PROFILE", "default") + t.Setenv("VERDA_SHARED_CREDENTIALS_FILE", filepath.Join(t.TempDir(), "absent")) + t.Setenv("VERDA_S3_ACCESS_KEY", "REPLACE_ME_ENV_KEY") + t.Setenv("VERDA_S3_SECRET_KEY", "REPLACE_ME_ENV_SECRET") + t.Setenv("VERDA_S3_ENDPOINT", "https://env.example.invalid") + + creds, err := loadCredsFromFactory(s3TestFactory()) + if err != nil { + t.Fatalf("loadCredsFromFactory: %v", err) + } + if got := resolveEndpoint(creds, "https://flag.example.invalid"); got != "https://flag.example.invalid" { + t.Errorf("resolveEndpoint = %q, want the flag value to win over env", got) + } + if got := resolveEndpoint(creds, ""); got != "https://env.example.invalid" { + t.Errorf("resolveEndpoint = %q, want the env value when no flag is passed", got) + } +} + +// redactedCreds keeps key material out of test failure output. +func redactedCreds(c *options.S3Credentials) map[string]any { + return map[string]any{ + "access_key_set": c.AccessKey != "", + "secret_key_set": c.SecretKey != "", + "endpoint": c.Endpoint, + "region": c.Region, + "auth_mode": c.AuthMode, + } +} diff --git a/internal/verda-cli/cmd/objectstorage/show.go b/internal/verda-cli/cmd/objectstorage/show.go index 2b86c11..2236e06 100644 --- a/internal/verda-cli/cmd/objectstorage/show.go +++ b/internal/verda-cli/cmd/objectstorage/show.go @@ -57,10 +57,15 @@ func NewCmdShow(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command { profile = "default" } - creds, err := options.LoadS3CredentialsForProfile(path, profile) + // Same resolver the transfer commands use, or show would report + // "not configured" for an env-only setup that `ls` handles fine. + creds, envApplied, err := options.ResolveS3Credentials(path, profile) _, _ = fmt.Fprintf(ioStreams.Out, "profile: %s\n", profile) _, _ = fmt.Fprintf(ioStreams.Out, "credentials_file: %s\n", path) + if len(envApplied) > 0 { + _, _ = fmt.Fprintf(ioStreams.Out, "env_overrides: %s\n", strings.Join(envApplied, ", ")) + } if err != nil { _, _ = fmt.Fprintf(ioStreams.Out, "s3_configured: false\n") diff --git a/internal/verda-cli/options/s3_credentials.go b/internal/verda-cli/options/s3_credentials.go index 12da7d0..d566abc 100644 --- a/internal/verda-cli/options/s3_credentials.go +++ b/internal/verda-cli/options/s3_credentials.go @@ -47,3 +47,47 @@ func LoadS3CredentialsForProfile(path, profile string) (*S3Credentials, error) { AuthMode: strings.TrimSpace(section.Key("verda_s3_auth_mode").String()), }, nil } + +// ApplyS3Env overlays VERDA_S3_* onto c field by field and reports which +// variables were applied (names only — never values; callers print these). +// Empty means unset, so `export VERDA_S3_REGION=` cannot blank a good profile. +func (c *S3Credentials) ApplyS3Env() []string { + overrides := []struct { + name string + field *string + }{ + {"VERDA_S3_ACCESS_KEY", &c.AccessKey}, + {"VERDA_S3_SECRET_KEY", &c.SecretKey}, + {"VERDA_S3_ENDPOINT", &c.Endpoint}, + {"VERDA_S3_REGION", &c.Region}, + {"VERDA_S3_AUTH_MODE", &c.AuthMode}, + } + + var applied []string + for _, o := range overrides { + if value := strings.TrimSpace(os.Getenv(o.name)); value != "" { + *o.field = value + applied = append(applied, o.name) + } + } + return applied +} + +// ResolveS3Credentials loads the profile from path, then overlays VERDA_S3_* +// per field: flags (applied later, in the S3 client) → env → file. A missing +// file or profile stops being fatal once env alone carries a complete set — +// that is the CI case, where writing secrets to disk is what we're avoiding. +// The returned credentials are never nil, even alongside an error. +func ResolveS3Credentials(path, profile string) (*S3Credentials, []string, error) { + creds, err := LoadS3CredentialsForProfile(path, profile) + if err != nil { + creds = &S3Credentials{} + } + + applied := creds.ApplyS3Env() + + if err != nil && !creds.HasCredentials() { + return creds, applied, err + } + return creds, applied, nil +} diff --git a/internal/verda-cli/options/s3_credentials_test.go b/internal/verda-cli/options/s3_credentials_test.go index e9a4957..daa5abb 100644 --- a/internal/verda-cli/options/s3_credentials_test.go +++ b/internal/verda-cli/options/s3_credentials_test.go @@ -99,3 +99,119 @@ verda_s3_access_key = AKIA123 t.Errorf("SecretKey = %q, want empty", creds.SecretKey) } } + +// ResolveS3Credentials layers env over the file per field. These tests own the +// unit-level contract; cmd/objectstorage/helper_test.go owns the wiring. +func TestResolveS3CredentialsEnvOnly(t *testing.T) { + // No t.Parallel: t.Setenv. + t.Setenv("VERDA_S3_ACCESS_KEY", "REPLACE_ME_ENV_KEY") + t.Setenv("VERDA_S3_SECRET_KEY", "REPLACE_ME_ENV_SECRET") + t.Setenv("VERDA_S3_ENDPOINT", "https://env.example.invalid") + t.Setenv("VERDA_S3_REGION", "eu-north-1") + t.Setenv("VERDA_S3_AUTH_MODE", "credentials") + + creds, applied, err := ResolveS3Credentials(filepath.Join(t.TempDir(), "absent"), "default") + if err != nil { + t.Fatalf("ResolveS3Credentials() error: %v", err) + } + if !creds.HasCredentials() { + t.Fatal("HasCredentials() = false for a complete env set") + } + if creds.Region != "eu-north-1" || creds.AuthMode != "credentials" { + t.Errorf("Region = %q, AuthMode = %q", creds.Region, creds.AuthMode) + } + if len(applied) != 5 { + t.Errorf("applied = %v, want all five variable names", applied) + } +} + +func TestResolveS3CredentialsPerFieldMerge(t *testing.T) { + path := filepath.Join(t.TempDir(), "credentials") + content := `[default] +verda_s3_access_key = FILE_KEY +verda_s3_secret_key = FILE_SECRET +verda_s3_endpoint = https://file.example.invalid +verda_s3_region = us-east-1 +` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("VERDA_S3_ENDPOINT", "https://env.example.invalid") + + creds, applied, err := ResolveS3Credentials(path, "default") + if err != nil { + t.Fatalf("ResolveS3Credentials() error: %v", err) + } + if creds.Endpoint != "https://env.example.invalid" { + t.Errorf("Endpoint = %q, want the env value", creds.Endpoint) + } + if creds.AccessKey != "FILE_KEY" || creds.SecretKey != "FILE_SECRET" || creds.Region != "us-east-1" { + t.Errorf("non-overridden fields lost: access_key_set=%t region=%q", creds.AccessKey != "", creds.Region) + } + if len(applied) != 1 || applied[0] != "VERDA_S3_ENDPOINT" { + t.Errorf("applied = %v, want [VERDA_S3_ENDPOINT]", applied) + } +} + +func TestResolveS3CredentialsEmptyEnvIsUnset(t *testing.T) { + path := filepath.Join(t.TempDir(), "credentials") + content := `[default] +verda_s3_access_key = FILE_KEY +verda_s3_secret_key = FILE_SECRET +verda_s3_endpoint = https://file.example.invalid +verda_s3_region = us-east-1 +` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("VERDA_S3_REGION", "") + t.Setenv("VERDA_S3_ENDPOINT", " ") + + creds, applied, err := ResolveS3Credentials(path, "default") + if err != nil { + t.Fatalf("ResolveS3Credentials() error: %v", err) + } + if creds.Region != "us-east-1" { + t.Errorf("Region = %q; empty env var blanked the file value", creds.Region) + } + if creds.Endpoint != "https://file.example.invalid" { + t.Errorf("Endpoint = %q; whitespace-only env var blanked the file value", creds.Endpoint) + } + if len(applied) != 0 { + t.Errorf("applied = %v, want none", applied) + } +} + +// A missing file with an incomplete env must still surface the load error, so +// callers keep their "not configured" path. +func TestResolveS3CredentialsMissingFileIncompleteEnv(t *testing.T) { + t.Setenv("VERDA_S3_ACCESS_KEY", "REPLACE_ME_ENV_KEY") + + creds, applied, err := ResolveS3Credentials(filepath.Join(t.TempDir(), "absent"), "default") + if err == nil { + t.Fatal("expected the file-load error to survive an incomplete env") + } + if creds == nil { + t.Fatal("creds must never be nil, even with an error") + } + if creds.HasCredentials() { + t.Error("HasCredentials() = true with only an access key") + } + if len(applied) != 1 { + t.Errorf("applied = %v, want [VERDA_S3_ACCESS_KEY]", applied) + } +} + +func TestResolveS3CredentialsEmptyPathEnvOnly(t *testing.T) { + t.Setenv("VERDA_S3_ACCESS_KEY", "REPLACE_ME_ENV_KEY") + t.Setenv("VERDA_S3_SECRET_KEY", "REPLACE_ME_ENV_SECRET") + t.Setenv("VERDA_S3_ENDPOINT", "https://env.example.invalid") + + creds, _, err := ResolveS3Credentials("", "default") + if err != nil { + t.Fatalf("ResolveS3Credentials(\"\") error: %v", err) + } + if !creds.HasCredentials() { + t.Error("HasCredentials() = false; env-only resolution with no file path failed") + } +} From aa68b72bf12db73bdad9100e723a154b2e0a7939 Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 15:13:59 +0300 Subject: [PATCH 03/18] fix(sshkey,startupscript): accept an optional positional id on delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vm delete and volume delete both take an optional positional id plus --id, but ssh-key delete and startup-script delete were cobra.NoArgs, so `verda ssh-key delete ` failed with `unknown command`. Scripts had to special-case these two commands. Switch both to MaximumNArgs(1) and copy the established shape. --id keeps working unchanged. Passing both an argument and --id is a usage error rather than a silent pick — vm's shortcut lets the positional overwrite --id, which hides a typo that targets the wrong resource. The agent-mode --yes guard still fires before any API call, whichever way the id arrived. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/sshkey/delete.go | 20 +++-- internal/verda-cli/cmd/sshkey/delete_test.go | 77 +++++++++++++++++++ .../verda-cli/cmd/startupscript/delete.go | 20 +++-- .../cmd/startupscript/delete_test.go | 77 +++++++++++++++++++ 4 files changed, 182 insertions(+), 12 deletions(-) diff --git a/internal/verda-cli/cmd/sshkey/delete.go b/internal/verda-cli/cmd/sshkey/delete.go index c8ae596..59bb953 100644 --- a/internal/verda-cli/cmd/sshkey/delete.go +++ b/internal/verda-cli/cmd/sshkey/delete.go @@ -35,31 +35,39 @@ func NewCmdDelete(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command opts := &deleteOptions{} cmd := &cobra.Command{ - Use: "delete", + Use: "delete []", Aliases: []string{"rm"}, Short: "Delete an SSH key", Long: cmdutil.LongDesc(` Delete an SSH key from your account. In interactive mode you will be - prompted to select a key and confirm deletion. Use --id for - non-interactive use. Agent mode requires --id and --yes. + prompted to select a key and confirm deletion. Pass the key id as an + argument or with --id for non-interactive use. Agent mode requires an + id and --yes. `), Example: cmdutil.Examples(` # Interactive verda ssh-key delete # Non-interactive + verda ssh-key delete abc-123 verda ssh-key delete --id abc-123 # Agent mode - verda --agent ssh-key delete --id abc-123 --yes + verda --agent ssh-key delete abc-123 --yes `), - Args: cobra.NoArgs, + Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if len(args) > 0 { + if opts.ID != "" { + return cmdutil.UsageErrorf(cmd, "pass the SSH key id either as an argument or with --id, not both") + } + opts.ID = args[0] + } return runDelete(cmd, f, ioStreams, opts) }, } - cmd.Flags().StringVar(&opts.ID, "id", "", "SSH key ID to delete") + cmd.Flags().StringVar(&opts.ID, "id", "", "SSH key ID to delete (alternative to the positional argument)") cmd.Flags().BoolVar(&opts.Yes, "yes", false, "Skip confirmation for destructive actions (required in agent mode)") return cmd diff --git a/internal/verda-cli/cmd/sshkey/delete_test.go b/internal/verda-cli/cmd/sshkey/delete_test.go index 39aa87f..804c921 100644 --- a/internal/verda-cli/cmd/sshkey/delete_test.go +++ b/internal/verda-cli/cmd/sshkey/delete_test.go @@ -16,6 +16,8 @@ package sshkey import ( "bytes" + "errors" + "strings" "testing" "github.com/spf13/cobra" @@ -68,3 +70,78 @@ func TestDeleteHasYesFlag(t *testing.T) { t.Error("delete missing --yes flag") } } + +// deleteCmdErr runs `ssh-key delete` with args and returns the error. No client +// is configured, so a run that gets as far as resolving one returns ErrNoClient +// — which is exactly how we prove argument parsing succeeded. +func deleteCmdErr(t *testing.T, agent bool, args ...string) error { + t.Helper() + + var buf bytes.Buffer + ioStreams := cmdutil.IOStreams{Out: &buf, ErrOut: &buf} + f := &cmdutil.TestFactory{AgentModeOverride: agent} + + root := &cobra.Command{Use: "verda", SilenceUsage: true, SilenceErrors: true} + root.AddCommand(NewCmdSSHKey(f, ioStreams)) + root.SetArgs(append([]string{"ssh-key", "delete"}, args...)) + return root.Execute() +} + +// vm delete and volume delete both take an optional positional id; these two +// commands were --id only, so scripts could not use one calling convention. +func TestDeleteAcceptsPositionalID(t *testing.T) { + t.Parallel() + + err := deleteCmdErr(t, false, "key-123", "--yes") + if !errors.Is(err, cmdutil.ErrNoClient) { + t.Fatalf("err = %v, want ErrNoClient (the positional id was rejected before the API)", err) + } +} + +// --id is published; it must keep working exactly as before. +func TestDeleteStillAcceptsIDFlag(t *testing.T) { + t.Parallel() + + err := deleteCmdErr(t, false, "--id", "key-123", "--yes") + if !errors.Is(err, cmdutil.ErrNoClient) { + t.Fatalf("err = %v, want ErrNoClient", err) + } +} + +// Two ids in one invocation is a typo, not an intent — refuse rather than +// silently picking one (vm's shortcut lets the positional win; not copied). +func TestDeleteRejectsPositionalAndFlagTogether(t *testing.T) { + t.Parallel() + + err := deleteCmdErr(t, false, "key-123", "--id", "key-456", "--yes") + if err == nil { + t.Fatal("expected a usage error when both a positional id and --id are given") + } + if errors.Is(err, cmdutil.ErrNoClient) { + t.Fatal("conflicting ids reached the API layer; must fail before that") + } + if !strings.Contains(err.Error(), "--id") { + t.Errorf("error should name the conflicting flag, got: %v", err) + } +} + +// The agent guard must still fire before any API call when the id is positional. +func TestDeleteAgentModePositionalRequiresYes(t *testing.T) { + t.Parallel() + + err := deleteCmdErr(t, true, "key-123") + if err == nil { + t.Fatal("expected error: agent mode delete requires --yes") + } + if ae := cmdutil.ClassifyError(err); ae.Code != "CONFIRMATION_REQUIRED" { + t.Fatalf("code = %q, want CONFIRMATION_REQUIRED (err: %v)", ae.Code, err) + } +} + +func TestDeleteRejectsTwoPositionals(t *testing.T) { + t.Parallel() + + if err := deleteCmdErr(t, false, "key-123", "key-456", "--yes"); err == nil { + t.Fatal("expected an error for two positional ids") + } +} diff --git a/internal/verda-cli/cmd/startupscript/delete.go b/internal/verda-cli/cmd/startupscript/delete.go index 623730d..5af6d9b 100644 --- a/internal/verda-cli/cmd/startupscript/delete.go +++ b/internal/verda-cli/cmd/startupscript/delete.go @@ -35,31 +35,39 @@ func NewCmdDelete(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command opts := &deleteOptions{} cmd := &cobra.Command{ - Use: "delete", + Use: "delete []", Aliases: []string{"rm"}, Short: "Delete a startup script", Long: cmdutil.LongDesc(` Delete a startup script from your account. In interactive mode you - will be prompted to select a script and confirm deletion. Use --id - for non-interactive use. Agent mode requires --id and --yes. + will be prompted to select a script and confirm deletion. Pass the + script id as an argument or with --id for non-interactive use. Agent + mode requires an id and --yes. `), Example: cmdutil.Examples(` # Interactive verda startup-script delete # Non-interactive + verda startup-script delete abc-123 verda startup-script delete --id abc-123 # Agent mode - verda --agent startup-script delete --id abc-123 --yes + verda --agent startup-script delete abc-123 --yes `), - Args: cobra.NoArgs, + Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if len(args) > 0 { + if opts.ID != "" { + return cmdutil.UsageErrorf(cmd, "pass the startup script id either as an argument or with --id, not both") + } + opts.ID = args[0] + } return runDelete(cmd, f, ioStreams, opts) }, } - cmd.Flags().StringVar(&opts.ID, "id", "", "Startup script ID to delete") + cmd.Flags().StringVar(&opts.ID, "id", "", "Startup script ID to delete (alternative to the positional argument)") cmd.Flags().BoolVar(&opts.Yes, "yes", false, "Skip confirmation for destructive actions (required in agent mode)") return cmd diff --git a/internal/verda-cli/cmd/startupscript/delete_test.go b/internal/verda-cli/cmd/startupscript/delete_test.go index 80bcebe..957552a 100644 --- a/internal/verda-cli/cmd/startupscript/delete_test.go +++ b/internal/verda-cli/cmd/startupscript/delete_test.go @@ -16,6 +16,8 @@ package startupscript import ( "bytes" + "errors" + "strings" "testing" "github.com/spf13/cobra" @@ -68,3 +70,78 @@ func TestDeleteHasYesFlag(t *testing.T) { t.Error("delete missing --yes flag") } } + +// deleteCmdErr runs `startup-script delete` with args and returns the error. No +// client is configured, so a run that gets as far as resolving one returns +// ErrNoClient — which is exactly how we prove argument parsing succeeded. +func deleteCmdErr(t *testing.T, agent bool, args ...string) error { + t.Helper() + + var buf bytes.Buffer + ioStreams := cmdutil.IOStreams{Out: &buf, ErrOut: &buf} + f := &cmdutil.TestFactory{AgentModeOverride: agent} + + root := &cobra.Command{Use: "verda", SilenceUsage: true, SilenceErrors: true} + root.AddCommand(NewCmdStartupScript(f, ioStreams)) + root.SetArgs(append([]string{"startup-script", "delete"}, args...)) + return root.Execute() +} + +// vm delete and volume delete both take an optional positional id; these two +// commands were --id only, so scripts could not use one calling convention. +func TestDeleteAcceptsPositionalID(t *testing.T) { + t.Parallel() + + err := deleteCmdErr(t, false, "script-123", "--yes") + if !errors.Is(err, cmdutil.ErrNoClient) { + t.Fatalf("err = %v, want ErrNoClient (the positional id was rejected before the API)", err) + } +} + +// --id is published; it must keep working exactly as before. +func TestDeleteStillAcceptsIDFlag(t *testing.T) { + t.Parallel() + + err := deleteCmdErr(t, false, "--id", "script-123", "--yes") + if !errors.Is(err, cmdutil.ErrNoClient) { + t.Fatalf("err = %v, want ErrNoClient", err) + } +} + +// Two ids in one invocation is a typo, not an intent — refuse rather than +// silently picking one (vm's shortcut lets the positional win; not copied). +func TestDeleteRejectsPositionalAndFlagTogether(t *testing.T) { + t.Parallel() + + err := deleteCmdErr(t, false, "script-123", "--id", "script-456", "--yes") + if err == nil { + t.Fatal("expected a usage error when both a positional id and --id are given") + } + if errors.Is(err, cmdutil.ErrNoClient) { + t.Fatal("conflicting ids reached the API layer; must fail before that") + } + if !strings.Contains(err.Error(), "--id") { + t.Errorf("error should name the conflicting flag, got: %v", err) + } +} + +// The agent guard must still fire before any API call when the id is positional. +func TestDeleteAgentModePositionalRequiresYes(t *testing.T) { + t.Parallel() + + err := deleteCmdErr(t, true, "script-123") + if err == nil { + t.Fatal("expected error: agent mode delete requires --yes") + } + if ae := cmdutil.ClassifyError(err); ae.Code != "CONFIRMATION_REQUIRED" { + t.Fatalf("code = %q, want CONFIRMATION_REQUIRED (err: %v)", ae.Code, err) + } +} + +func TestDeleteRejectsTwoPositionals(t *testing.T) { + t.Parallel() + + if err := deleteCmdErr(t, false, "script-123", "script-456", "--yes"); err == nil { + t.Fatal("expected an error for two positional ids") + } +} From d73ac58088919b986bc9df1eb5536d6b719e5cb7 Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 15:20:21 +0300 Subject: [PATCH 04/18] style(vm): name the delete action-name literal for goconst Pre-existing violation, not introduced here: "delete" appears 12 times in cmd/vm (1 in batch.go, 11 in tests). goconst counts across the package but only reports on non-test files, so it lands on batch.go:278. golangci-lint's cache had been hiding it; any edit to the package surfaces it and fails make lint. Reported separately from the help-text change it blocked. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/vm/batch.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/verda-cli/cmd/vm/batch.go b/internal/verda-cli/cmd/vm/batch.go index f398bfc..17e7631 100644 --- a/internal/verda-cli/cmd/vm/batch.go +++ b/internal/verda-cli/cmd/vm/batch.go @@ -264,6 +264,10 @@ func confirmBatchDelete(ctx context.Context, f cmdutil.Factory, ioStreams cmduti return confirmed, deleteVolumes, nil } +// actionNameDelete is the CLI spelling of the delete action, distinct from the +// SDK's verda.ActionDelete wire value. +const actionNameDelete = "delete" + // actionNameToAPI maps CLI action names to SDK action constants. func actionNameToAPI(action string) string { switch strings.ToLower(action) { @@ -275,7 +279,7 @@ func actionNameToAPI(action string) string { return verda.ActionForceShutdown case "hibernate": return verda.ActionHibernate - case "delete": + case actionNameDelete: return verda.ActionDelete default: return action From 03d45e592939543bbaab829e71a9a6e86c143e2f Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 15:20:59 +0300 Subject: [PATCH 05/18] docs(help): state the four trapdoors the help text hid Text only, no behavior change. instance-types said "List all available instance types", which reads as live stock and is the reading users act on. It is a catalog; point at verda availability for what can actually be deployed. --os-volume-name and --os-volume-on-spot-discontinue now state that they require --os-volume-size (enforced at create.go:351, previously only discoverable by hitting the error). --description states the 100-character cap, which until now surfaced only as an API 400. No client-side validation added on purpose: a client-side cap that drifts from the API is worse than the 400. --with-volumes states that without it the OS volume survives detached and keeps billing. Updated in both places it is defined (shortcuts.go and action.go) so the two surfaces cannot disagree. Also documents the new positional delete id in the ssh-key and startup-script READMEs, completing aa68b72. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/instancetypes/instancetypes.go | 10 +++++++--- internal/verda-cli/cmd/sshkey/README.md | 11 ++++++----- internal/verda-cli/cmd/startupscript/README.md | 9 +++++---- internal/verda-cli/cmd/vm/action.go | 2 +- internal/verda-cli/cmd/vm/create.go | 8 ++++---- internal/verda-cli/cmd/vm/shortcuts.go | 2 +- 6 files changed, 24 insertions(+), 18 deletions(-) diff --git a/internal/verda-cli/cmd/instancetypes/instancetypes.go b/internal/verda-cli/cmd/instancetypes/instancetypes.go index 85bb939..7ca932f 100644 --- a/internal/verda-cli/cmd/instancetypes/instancetypes.go +++ b/internal/verda-cli/cmd/instancetypes/instancetypes.go @@ -39,10 +39,14 @@ func NewCmdInstanceTypes(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra. cmd := &cobra.Command{ Use: "instance-types", Aliases: []string{"types"}, - Short: "List available instance types with specs and pricing", + Short: "List the instance type catalog with specs and pricing", Long: cmdutil.LongDesc(` - List all available instance types with their specifications - and pricing. Filter by GPU or CPU to narrow results. + List every instance type Verda offers, with specifications and pricing. + Filter by GPU or CPU to narrow results. + + This is the catalog, not live stock: a type listed here may have no + capacity in any location right now. Run "verda availability" for what + is actually deployable, per location. `), Example: cmdutil.Examples(` # All instance types diff --git a/internal/verda-cli/cmd/sshkey/README.md b/internal/verda-cli/cmd/sshkey/README.md index 92305e5..4bac551 100644 --- a/internal/verda-cli/cmd/sshkey/README.md +++ b/internal/verda-cli/cmd/sshkey/README.md @@ -6,7 +6,7 @@ |---------|-------------|-----------| | `verda ssh-key list` | List all SSH keys (Name, ID, Fingerprint) | _(none)_ | | `verda ssh-key add` | Add an SSH key to your account | `--name`, `--public-key` | -| `verda ssh-key delete` | Delete an SSH key from your account | `--id`, `--yes` | +| `verda ssh-key delete []` | Delete an SSH key from your account | `--id`, `--yes` | ## Usage Examples @@ -30,11 +30,12 @@ verda ssh-key add --name my-key --public-key "ssh-ed25519 AAAA..." # Interactive (select from list, then confirm) verda ssh-key delete -# Non-interactive +# Non-interactive (positional id, or --id -- not both) +verda ssh-key delete abc-123 verda ssh-key delete --id abc-123 # Agent mode (structured result, no prompts) -verda --agent ssh-key delete --id abc-123 --yes +verda --agent ssh-key delete abc-123 --yes ``` ## Interactive vs Non-Interactive @@ -42,10 +43,10 @@ verda --agent ssh-key delete --id abc-123 --yes | Command | Non-interactive flags | Prompted when missing | |---------|----------------------|----------------------| | `add` | `--name`, `--public-key` | Name via text input, public key via text input | -| `delete` | `--id`, `--yes` | Fetches all keys, presents select list, then confirms | +| `delete` | positional `` or `--id`, plus `--yes` | Fetches all keys, presents select list, then confirms | | `list` | _(always non-interactive)_ | N/A | -Destructive deletes ask for confirmation interactively; pass `--yes` to skip it. In agent mode (`--agent`), `--id` and `--yes` are required — without `--yes` the command fails with `CONFIRMATION_REQUIRED`, and success prints a structured JSON result. +Destructive deletes ask for confirmation interactively; pass `--yes` to skip it. In agent mode (`--agent`), an id and `--yes` are required — without `--yes` the command fails with `CONFIRMATION_REQUIRED`, and success prints a structured JSON result. ## Architecture Notes diff --git a/internal/verda-cli/cmd/startupscript/README.md b/internal/verda-cli/cmd/startupscript/README.md index b9a2ef3..43cb91b 100644 --- a/internal/verda-cli/cmd/startupscript/README.md +++ b/internal/verda-cli/cmd/startupscript/README.md @@ -6,7 +6,7 @@ |---------|-------------|-----------| | `verda startup-script list` | List all startup scripts (Name, ID, Created) | _(none)_ | | `verda startup-script add` | Add a startup script | `--name`, `--file`, `--script` | -| `verda startup-script delete` | Delete a startup script | `--id`, `--yes` | +| `verda startup-script delete []` | Delete a startup script | `--id`, `--yes` | ## Usage Examples @@ -34,10 +34,11 @@ verda startup-script add --name setup --script "#!/bin/bash\napt update" verda startup-script delete # Non-interactive +verda startup-script delete abc-123 verda startup-script delete --id abc-123 # Agent mode (structured result, no prompts) -verda --agent startup-script delete --id abc-123 --yes +verda --agent startup-script delete abc-123 --yes ``` ## Interactive vs Non-Interactive @@ -45,10 +46,10 @@ verda --agent startup-script delete --id abc-123 --yes | Command | Non-interactive flags | Prompted when missing | |---------|----------------------|----------------------| | `add` | `--name`, `--file` or `--script` | Name via text input; script source via select ("Load from file" / "Paste content") | -| `delete` | `--id`, `--yes` | Fetches all scripts, presents select list, then confirms | +| `delete` | positional `` or `--id`, plus `--yes` | Fetches all scripts, presents select list, then confirms | | `list` | _(always non-interactive)_ | N/A | -Destructive deletes ask for confirmation interactively; pass `--yes` to skip it. In agent mode (`--agent`), `--id` and `--yes` are required — without `--yes` the command fails with `CONFIRMATION_REQUIRED`, and success prints a structured JSON result. +Destructive deletes ask for confirmation interactively; pass `--yes` to skip it. In agent mode (`--agent`), an id and `--yes` are required — without `--yes` the command fails with `CONFIRMATION_REQUIRED`, and success prints a structured JSON result. ## Architecture Notes diff --git a/internal/verda-cli/cmd/vm/action.go b/internal/verda-cli/cmd/vm/action.go index 05f96c2..5d63aaf 100644 --- a/internal/verda-cli/cmd/vm/action.go +++ b/internal/verda-cli/cmd/vm/action.go @@ -156,7 +156,7 @@ func NewCmdAction(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command cmd.Flags().StringVar(&opts.InstanceID, "id", "", "Instance ID to act on") cmd.Flags().StringVar(&opts.Action, "action", "", "Action to perform: start, shutdown, force_shutdown, hibernate, delete") cmd.Flags().BoolVar(&opts.Yes, "yes", false, "Skip confirmation for destructive actions (required in agent mode)") - cmd.Flags().BoolVar(&opts.WithVolumes, "with-volumes", false, "Also delete all attached volumes (delete only)") + cmd.Flags().BoolVar(&opts.WithVolumes, "with-volumes", false, "Also delete all attached volumes (delete only); without it the OS volume survives detached and keeps billing") // Hidden like on non-delete shortcuts: `vm delete --with-volumes` is the // canonical UX; the flag works here for `--action delete` (agent parity). _ = cmd.Flags().MarkHidden("with-volumes") diff --git a/internal/verda-cli/cmd/vm/create.go b/internal/verda-cli/cmd/vm/create.go index 7f10134..1d57bff 100644 --- a/internal/verda-cli/cmd/vm/create.go +++ b/internal/verda-cli/cmd/vm/create.go @@ -172,7 +172,7 @@ func NewCmdCreate(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command flags.StringVar(&opts.Image, "os", "", "OS image slug or an existing detached OS volume ID") flags.StringVar(&opts.Image, "image", "", "Alias of --os") flags.StringVar(&opts.Hostname, "hostname", "", "Hostname for the new VM") - flags.StringVar(&opts.Description, "description", "", "Human-readable description; defaults to the hostname") + flags.StringVar(&opts.Description, "description", "", "Human-readable description, max 100 characters; defaults to the hostname") flags.StringSliceVar(&opts.SSHKeyIDs, "ssh-key", nil, "SSH key ID to inject into the instance; repeat the flag for multiple keys") flags.StringSliceVar(&opts.SSHKeyIDs, "ssh-key-id", nil, "Alias of --ssh-key") flags.StringVar(&opts.LocationCode, "location", opts.LocationCode, "Location code, for example FIN-01") @@ -185,9 +185,9 @@ func NewCmdCreate(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command flags.BoolVar(&opts.IsSpot, "is-spot", false, "Request a spot instance") flags.BoolVar(&opts.IsSpot, "spot", false, "Alias of --is-spot") flags.StringVar(&opts.Coupon, "coupon", "", "Coupon code to apply to the instance creation") - flags.StringVar(&opts.OSVolumeName, "os-volume-name", "", "Name of the OS volume to create") - flags.IntVar(&opts.OSVolumeSize, "os-volume-size", 0, "Size of the OS volume in GiB") - flags.StringVar(&opts.OSVolumeOnSpotDiscontinue, "os-volume-on-spot-discontinue", "", "Spot discontinue policy for the OS volume: keep_detached, move_to_trash, or delete_permanently") + flags.StringVar(&opts.OSVolumeName, "os-volume-name", "", "Name of the OS volume to create; requires --os-volume-size") + flags.IntVar(&opts.OSVolumeSize, "os-volume-size", 0, "Size of the OS volume in GiB; required if any other --os-volume-* flag is set") + flags.StringVar(&opts.OSVolumeOnSpotDiscontinue, "os-volume-on-spot-discontinue", "", "Spot discontinue policy for the OS volume: keep_detached, move_to_trash, or delete_permanently; requires --os-volume-size and --is-spot") flags.StringVar(&opts.StorageName, "storage-name", "", "Name of the optional additional storage volume; defaults to -storage") flags.IntVar(&opts.StorageSize, "storage-size", 0, "Size of the optional additional storage volume in GiB") flags.StringVar(&opts.StorageType, "storage-type", opts.StorageType, "Type of the optional additional storage volume") diff --git a/internal/verda-cli/cmd/vm/shortcuts.go b/internal/verda-cli/cmd/vm/shortcuts.go index 327b724..73c505e 100644 --- a/internal/verda-cli/cmd/vm/shortcuts.go +++ b/internal/verda-cli/cmd/vm/shortcuts.go @@ -82,7 +82,7 @@ func newShortcutCmd(f cmdutil.Factory, ioStreams cmdutil.IOStreams, def shortcut cmd.Flags().BoolVar(&opts.All, "all", false, "Target all instances (use with --status/--hostname to filter)") cmd.Flags().StringVar(&opts.Status, "status", "", "Filter by status, requires --all (e.g., running, offline)") cmd.Flags().StringVar(&opts.Hostname, "hostname", "", "Filter by hostname glob pattern, requires --all (e.g., \"test-*\")") - cmd.Flags().BoolVar(&opts.WithVolumes, "with-volumes", false, "Also delete all attached volumes (delete only)") + cmd.Flags().BoolVar(&opts.WithVolumes, "with-volumes", false, "Also delete all attached volumes (delete only); without it the OS volume survives detached and keeps billing") opts.Wait.AddFlags(cmd.Flags(), true) if def.Action != verda.ActionDelete { From c5c9c93d979c9ce5b9ca1cfbcd1a96dafa13f9ce Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 15:43:37 +0300 Subject: [PATCH 06/18] test: cover the surfaces the hotfix changed but left untested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverage audit of the four fixes found three gaps, all closed here: object-storage show read credentials through its own call site, and no test exercised the env path there — an env-only setup reporting "not configured" would have gone unnoticed. Three tests: env-only is reported configured, env_overrides names variables and never values (with per-field merge asserted), and nothing-configured still says so. Every S3 verb (ls/cp/mv/sync/rm/mb/rb/presign) shares buildClientDefault, but every existing test swaps in a fake client, so the real builder's env path was never executed. Two tests call it directly: env-only credentials build a client, and no credentials keep the configure hint. MCP add_ssh_key returns the created key to the agent — the second place a zero timestamp reaches a reaper — and only list_ssh_keys was covered. Verified non-vacuous by mutation: disabling the env overlay fails 10 tests (4 new), and making the view emit a zero time fails 7 across cmd/util, cmd/mcp and cmd/sshkey, including the new add test. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/mcp/tools_ssh_test.go | 58 +++++++++++++ .../cmd/objectstorage/helper_test.go | 38 +++++++++ .../verda-cli/cmd/objectstorage/show_test.go | 82 +++++++++++++++++++ 3 files changed, 178 insertions(+) diff --git a/internal/verda-cli/cmd/mcp/tools_ssh_test.go b/internal/verda-cli/cmd/mcp/tools_ssh_test.go index 521cb7b..af20a01 100644 --- a/internal/verda-cli/cmd/mcp/tools_ssh_test.go +++ b/internal/verda-cli/cmd/mcp/tools_ssh_test.go @@ -129,3 +129,61 @@ func TestListSSHKeysSearchStillFilters(t *testing.T) { t.Errorf("filtered result still emits a zero timestamp:\n%s", got) } } + +// add_ssh_key returns the created key to the agent, so it is the second place a +// zero timestamp can reach a reaper. AddSSHKey POSTs, gets a plain-text id back, +// then re-reads the key — both stubs are needed. +func TestAddSSHKeyOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + const keyID = "4d13391d-bdef-49ec-84de-53a2f6174905" + + mux := http.NewServeMux() + mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "access_token": "test-token", + "token_type": "Bearer", + }) + }) + mux.HandleFunc("POST /ssh-keys", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/plain") + _, _ = w.Write([]byte(keyID)) + }) + // Re-read after create: same shape as the list endpoint, no created_at. + mux.HandleFunc("GET /ssh-keys/"+keyID, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"id":"` + keyID + `","name":"meng",` + + `"key":"ssh-ed25519 AAAA","fingerprint":null}]`)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + client, err := verda.NewClient( + verda.WithBaseURL(srv.URL), + verda.WithClientID("test-id"), + verda.WithClientSecret("test-secret"), + ) + if err != nil { + t.Fatalf("creating client: %v", err) + } + + res, err := NewServer(client).handleAddSSHKey(context.Background(), callReq("add_ssh_key", map[string]any{ + "name": "meng", + "public_key": "ssh-ed25519 AAAA", + })) + if err != nil { + t.Fatalf("handleAddSSHKey: %v", err) + } + got := resultText(t, res) + + if strings.Contains(got, "0001-01-01") { + t.Errorf("add_ssh_key emits a zero timestamp as data:\n%s", got) + } + if strings.Contains(got, "created_at") { + t.Errorf("created_at present though the API never sent it:\n%s", got) + } + if !strings.Contains(got, keyID) { + t.Errorf("created key id missing from the result:\n%s", got) + } +} diff --git a/internal/verda-cli/cmd/objectstorage/helper_test.go b/internal/verda-cli/cmd/objectstorage/helper_test.go index d9dff55..1ecba38 100644 --- a/internal/verda-cli/cmd/objectstorage/helper_test.go +++ b/internal/verda-cli/cmd/objectstorage/helper_test.go @@ -243,3 +243,41 @@ func redactedCreds(c *options.S3Credentials) map[string]any { "auth_mode": c.AuthMode, } } + +// Every object-storage verb (ls/cp/mv/sync/rm/mb/rb/presign) builds its client +// through buildClientDefault, so this one test covers the env path for all of +// them. It calls the real builder, not the swapped fake. +func TestBuildClientDefaultResolvesEnvCredentials(t *testing.T) { + // No t.Parallel: t.Setenv. + t.Setenv("VERDA_PROFILE", "default") + t.Setenv("VERDA_SHARED_CREDENTIALS_FILE", filepath.Join(t.TempDir(), "absent")) + t.Setenv("VERDA_S3_ACCESS_KEY", "REPLACE_ME_ENV_KEY") + t.Setenv("VERDA_S3_SECRET_KEY", "REPLACE_ME_ENV_SECRET") + t.Setenv("VERDA_S3_ENDPOINT", "https://env.example.invalid") + + client, err := buildClientDefault(context.Background(), s3TestFactory(), ClientOverrides{}) + if err != nil { + t.Fatalf("buildClientDefault with env-only credentials: %v", err) + } + if client == nil { + t.Fatal("client is nil") + } +} + +// Same funnel, nothing configured: the friendly hint must survive. +func TestBuildClientDefaultWithoutCredentialsStillHints(t *testing.T) { + // No t.Parallel: t.Setenv. + t.Setenv("VERDA_PROFILE", "default") + t.Setenv("VERDA_SHARED_CREDENTIALS_FILE", filepath.Join(t.TempDir(), "absent")) + for _, v := range []string{"VERDA_S3_ACCESS_KEY", "VERDA_S3_SECRET_KEY", "VERDA_S3_ENDPOINT"} { + t.Setenv(v, "") + } + + _, err := buildClientDefault(context.Background(), s3TestFactory(), ClientOverrides{}) + if err == nil { + t.Fatal("expected the 'no S3 credentials configured' error") + } + if !strings.Contains(err.Error(), "object-storage configure") { + t.Errorf("error lost the configure hint: %v", err) + } +} diff --git a/internal/verda-cli/cmd/objectstorage/show_test.go b/internal/verda-cli/cmd/objectstorage/show_test.go index 6bba7d4..40a05b7 100644 --- a/internal/verda-cli/cmd/objectstorage/show_test.go +++ b/internal/verda-cli/cmd/objectstorage/show_test.go @@ -147,3 +147,85 @@ verda_s3_region = eu-west-1 t.Errorf("staging show leaked the default profile's endpoint:\n%s", stdout) } } + +// show must report what the transfer commands will actually use. Before env +// support it read the file only, so an env-only setup that `ls` handles fine +// showed up here as "not configured". +func TestShow_EnvOnlyIsReportedConfigured(t *testing.T) { + // no t.Parallel — t.Setenv + t.Setenv("VERDA_S3_ACCESS_KEY", "REPLACE_ME_ENV_KEY") + t.Setenv("VERDA_S3_SECRET_KEY", "REPLACE_ME_ENV_SECRET") + t.Setenv("VERDA_S3_ENDPOINT", "https://env.example.invalid") + t.Setenv("VERDA_S3_REGION", "eu-north-1") + + stdout, stderr := runShow(t, filepath.Join(t.TempDir(), "absent")) + + for _, want := range []string{ + "access_key_loaded: true", + "secret_key_loaded: true", + "https://env.example.invalid", + "eu-north-1", + } { + if !strings.Contains(stdout, want) { + t.Errorf("stdout missing %q:\n%s", want, stdout) + } + } + if strings.Contains(stdout, "s3_configured: false") { + t.Errorf("env-only credentials reported as not configured:\n%s", stdout) + } + if strings.Contains(stderr, "No S3 credentials found") { + t.Errorf("unexpected 'not found' warning for an env-only setup:\n%s", stderr) + } +} + +// The env_overrides line names the variables in play and must never print a +// value — this is the one command whose whole job is explaining credentials. +func TestShow_EnvOverridesNamesVariablesNotValues(t *testing.T) { + // no t.Parallel — t.Setenv + path := writeCredsFile(t, `[default] +verda_s3_access_key = AKIA123 +verda_s3_secret_key = secret456 +verda_s3_endpoint = https://objects.example.com +verda_s3_region = eu-north-1 +`) + t.Setenv("VERDA_S3_ENDPOINT", "https://env.example.invalid") + + stdout, _ := runShow(t, path) + + if !strings.Contains(stdout, "env_overrides: VERDA_S3_ENDPOINT") { + t.Errorf("env_overrides line missing or wrong:\n%s", stdout) + } + if strings.Contains(stdout, "VERDA_S3_ACCESS_KEY") { + t.Errorf("listed a variable that was not set:\n%s", stdout) + } + // Per-field merge: the file's key material survives an endpoint override. + if !strings.Contains(stdout, "https://env.example.invalid") { + t.Errorf("endpoint not overridden by env:\n%s", stdout) + } + if !strings.Contains(stdout, "access_key_loaded: true") { + t.Errorf("file key material lost:\n%s", stdout) + } +} + +// No credentials anywhere must still read as not configured. +func TestShow_NoFileNoEnvStaysNotConfigured(t *testing.T) { + // no t.Parallel — t.Setenv + for _, v := range []string{ + "VERDA_S3_ACCESS_KEY", "VERDA_S3_SECRET_KEY", + "VERDA_S3_ENDPOINT", "VERDA_S3_REGION", "VERDA_S3_AUTH_MODE", + } { + t.Setenv(v, "") + } + + stdout, stderr := runShow(t, filepath.Join(t.TempDir(), "absent")) + + if !strings.Contains(stdout, "s3_configured: false") { + t.Errorf("expected s3_configured: false:\n%s", stdout) + } + if strings.Contains(stdout, "env_overrides:") { + t.Errorf("env_overrides printed with nothing set:\n%s", stdout) + } + if !strings.Contains(stderr, "No S3 credentials found") { + t.Errorf("expected the not-found hint on stderr:\n%s", stderr) + } +} From c27e9dfcc529f11de509dd2ecad2319f532585c6 Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 15:55:24 +0300 Subject: [PATCH 07/18] fix(output): stop emitting a zero created_at for instances and jobs Same defect as be842d4, found by auditing the surfaces rather than the diff, and worse: /v1/instances omits created_at too (the captured staging payload temp/docs/c1-ondemand-instance.json has no time-valued key at all), and verda.Instance.CreatedAt has no omitempty. So `vm list -o json` dated every instance to 0001-01-01, and an age-based reaper reading that deletes running instances, not SSH keys. Six surfaces, all agent-facing: vm list, vm describe, vm create --agent, MCP list_vms, MCP describe_vm, and serverless batchjob list (hidden feature; fixed so it does not ship with the defect). InstanceView mirrors all 24 verda.Instance fields explicitly rather than embedding the SDK struct: embedding plus a shadowed CreatedAt is correct in JSON but leaks `createdat: 0001-01-01T00:00:00Z` in YAML, because yaml.v3 inlines the embedded struct and ignores json tags. Verified both ways before choosing. TestInstanceViewCoversSDKFields compares json tag sets by reflection so the SDK cannot grow a field the view silently drops. Tests fail before and pass after: 8 tests across cmd/util, cmd/vm, cmd/mcp and cmd/serverless catch a mutation that makes the view emit the zero time. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/mcp/tools_vm.go | 4 +- .../cmd/mcp/tools_vm_created_at_test.go | 109 +++++++++++++++ .../verda-cli/cmd/serverless/batchjob_list.go | 2 +- .../batchjob_list_created_at_test.go | 78 +++++++++++ internal/verda-cli/cmd/util/views.go | 100 ++++++++++++++ internal/verda-cli/cmd/util/views_test.go | 125 ++++++++++++++++++ internal/verda-cli/cmd/vm/create.go | 2 +- internal/verda-cli/cmd/vm/describe.go | 2 +- internal/verda-cli/cmd/vm/list.go | 2 +- .../verda-cli/cmd/vm/list_created_at_test.go | 125 ++++++++++++++++++ 10 files changed, 543 insertions(+), 6 deletions(-) create mode 100644 internal/verda-cli/cmd/mcp/tools_vm_created_at_test.go create mode 100644 internal/verda-cli/cmd/serverless/batchjob_list_created_at_test.go create mode 100644 internal/verda-cli/cmd/vm/list_created_at_test.go diff --git a/internal/verda-cli/cmd/mcp/tools_vm.go b/internal/verda-cli/cmd/mcp/tools_vm.go index 304bfa5..3b3cf1a 100644 --- a/internal/verda-cli/cmd/mcp/tools_vm.go +++ b/internal/verda-cli/cmd/mcp/tools_vm.go @@ -219,7 +219,7 @@ func (s *Server) handleListVMs(ctx context.Context, req mcp.CallToolRequest) (*m if err != nil { return mcp.NewToolResultError(err.Error()), nil } - return jsonResult(instances) + return jsonResult(cmdutil.NewInstanceViews(instances)) } //nolint:gocritic // hugeParam: handler signature defined by mcp-go. @@ -238,7 +238,7 @@ func (s *Server) handleDescribeVM(ctx context.Context, req mcp.CallToolRequest) if err != nil { return mcp.NewToolResultError(err.Error()), nil } - return jsonResult(inst) + return jsonResult(cmdutil.NewInstanceView(inst)) } //nolint:gocritic,gocyclo // hugeParam + complexity from auto-resolving location/SSH keys. diff --git a/internal/verda-cli/cmd/mcp/tools_vm_created_at_test.go b/internal/verda-cli/cmd/mcp/tools_vm_created_at_test.go new file mode 100644 index 0000000..79c766f --- /dev/null +++ b/internal/verda-cli/cmd/mcp/tools_vm_created_at_test.go @@ -0,0 +1,109 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package mcp + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" +) + +// newInstanceServer wires an MCP Server to a stub replaying exact /instances +// bodies. /v1/instances omits created_at (see temp/docs/c1-ondemand-instance.json), +// and this is the agent surface, so a zero timestamp here is what an autonomous +// reaper would act on — against running instances. +func newInstanceServer(t *testing.T, listBody, oneBody string) *Server { + t.Helper() + + mux := http.NewServeMux() + mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "access_token": "test-token", + "token_type": "Bearer", + }) + }) + mux.HandleFunc("GET /instances", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(listBody)) + }) + mux.HandleFunc("GET /instances/inst-1", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(oneBody)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + client, err := verda.NewClient( + verda.WithBaseURL(srv.URL), + verda.WithClientID("test-id"), + verda.WithClientSecret("test-secret"), + ) + if err != nil { + t.Fatalf("creating client: %v", err) + } + return NewServer(client) +} + +func TestListVMsOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + body := `[{"id":"inst-1","hostname":"box-a","status":"running"},` + + `{"id":"inst-2","hostname":"box-b","status":"running","created_at":"2026-08-11T18:51:12Z"}]` + s := newInstanceServer(t, body, `{}`) + + res, err := s.handleListVMs(context.Background(), callReq("list_vms", nil)) + if err != nil { + t.Fatalf("handleListVMs: %v", err) + } + got := resultText(t, res) + + if strings.Contains(got, "0001-01-01") { + t.Errorf("MCP list_vms emits a zero timestamp as data:\n%s", got) + } + if !strings.Contains(got, "2026-08-11T18:51:12Z") { + t.Errorf("the dated instance lost its created_at:\n%s", got) + } + if strings.Count(got, "created_at") != 1 { + t.Errorf("created_at count = %d, want exactly 1:\n%s", strings.Count(got, "created_at"), got) + } + if !strings.Contains(got, "box-a") || !strings.Contains(got, "box-b") { + t.Errorf("instances dropped by the view:\n%s", got) + } +} + +func TestDescribeVMOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + s := newInstanceServer(t, `[]`, `{"id":"inst-1","hostname":"box-a","status":"running"}`) + + res, err := s.handleDescribeVM(context.Background(), callReq("describe_vm", map[string]any{"id": "inst-1"})) + if err != nil { + t.Fatalf("handleDescribeVM: %v", err) + } + got := resultText(t, res) + + if strings.Contains(got, "0001-01-01") { + t.Errorf("MCP describe_vm emits a zero timestamp as data:\n%s", got) + } + if !strings.Contains(got, "box-a") { + t.Errorf("hostname missing from the result:\n%s", got) + } +} diff --git a/internal/verda-cli/cmd/serverless/batchjob_list.go b/internal/verda-cli/cmd/serverless/batchjob_list.go index 372c157..b1d311b 100644 --- a/internal/verda-cli/cmd/serverless/batchjob_list.go +++ b/internal/verda-cli/cmd/serverless/batchjob_list.go @@ -57,7 +57,7 @@ func runBatchjobList(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IO cmdutil.DebugJSON(ioStreams.ErrOut, f.Debug(), "Jobs:", jobs) - if wrote, werr := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), jobs); wrote { + if wrote, werr := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), cmdutil.NewJobDeploymentShortViews(jobs)); wrote { return werr } diff --git a/internal/verda-cli/cmd/serverless/batchjob_list_created_at_test.go b/internal/verda-cli/cmd/serverless/batchjob_list_created_at_test.go new file mode 100644 index 0000000..6efb65c --- /dev/null +++ b/internal/verda-cli/cmd/serverless/batchjob_list_created_at_test.go @@ -0,0 +1,78 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package serverless + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" +) + +// JobDeploymentShortInfo carries a time.Time CreatedAt, so a deployment the API +// sent no created_at for used to be reported as created on 0001-01-01. +func TestBatchjobListOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + mux := http.NewServeMux() + mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "access_token": "test-token", + "token_type": "Bearer", + }) + }) + mux.HandleFunc("GET /job-deployments", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"name":"job-a"},` + + `{"name":"job-b","created_at":"2026-08-11T18:51:12Z"}]`)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + var stdout, stderr bytes.Buffer + f := newTestFactory(t, srv.URL) + cmd := newCmdBatchjobList(f, cmdutil.IOStreams{Out: &stdout, ErrOut: &stderr}) + cmd.SetArgs(nil) + if err := cmd.Execute(); err != nil { + t.Fatalf("batchjob list: %v\nstderr:\n%s", err, stderr.String()) + } + + out := stdout.String() + if strings.Contains(out, "0001-01-01") { + t.Errorf("batchjob list emits a zero timestamp as data:\n%s", out) + } + + var got []map[string]any + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + if len(got) != 2 { + t.Fatalf("len = %d, want 2:\n%s", len(got), out) + } + if _, ok := got[0]["created_at"]; ok { + t.Errorf("undated deployment carries created_at: %v", got[0]) + } + if got[1]["created_at"] != "2026-08-11T18:51:12Z" { + t.Errorf("dated deployment lost created_at: %v", got[1]) + } + if got[0]["name"] != "job-a" { + t.Errorf("name dropped by the view: %v", got[0]) + } +} diff --git a/internal/verda-cli/cmd/util/views.go b/internal/verda-cli/cmd/util/views.go index 13c9beb..0c29949 100644 --- a/internal/verda-cli/cmd/util/views.go +++ b/internal/verda-cli/cmd/util/views.go @@ -96,6 +96,106 @@ func NewStartupScriptViews(scripts []verda.StartupScript) []StartupScriptView { return views } +// InstanceView is the JSON/YAML shape for one instance. Every field of +// verda.Instance is mirrored with its own json key — TestInstanceViewCoversSDKFields +// fails if the SDK grows a field this view would silently drop. +// +// Nested types (CPU, GPU, …) are reused from the SDK: they carry no zero-time +// field, so there is nothing to omit, and re-declaring them would be four more +// structs to keep in sync. +type InstanceView struct { + ID string `json:"id" yaml:"id"` + IP *string `json:"ip" yaml:"ip"` + Status string `json:"status" yaml:"status"` + CreatedAt *time.Time `json:"created_at,omitempty" yaml:"created_at,omitempty"` + CPU verda.InstanceCPU `json:"cpu" yaml:"cpu"` + GPU verda.InstanceGPU `json:"gpu" yaml:"gpu"` + GPUMemory verda.InstanceMemory `json:"gpu_memory" yaml:"gpu_memory"` + Memory verda.InstanceMemory `json:"memory" yaml:"memory"` + Storage verda.InstanceStorage `json:"storage" yaml:"storage"` + Hostname string `json:"hostname" yaml:"hostname"` + Description string `json:"description" yaml:"description"` + Location string `json:"location" yaml:"location"` + PricePerHour verda.FlexibleFloat `json:"price_per_hour" yaml:"price_per_hour"` + IsSpot bool `json:"is_spot" yaml:"is_spot"` + InstanceType string `json:"instance_type" yaml:"instance_type"` + Image string `json:"image" yaml:"image"` + OSName string `json:"os_name" yaml:"os_name"` + StartupScriptID *string `json:"startup_script_id" yaml:"startup_script_id"` + SSHKeyIDs []string `json:"ssh_key_ids" yaml:"ssh_key_ids"` + OSVolumeID *string `json:"os_volume_id" yaml:"os_volume_id"` + JupyterToken string `json:"jupyter_token" yaml:"jupyter_token"` + Contract string `json:"contract" yaml:"contract"` + Pricing string `json:"pricing" yaml:"pricing"` + VolumeIDs []string `json:"volume_ids" yaml:"volume_ids"` +} + +// NewInstanceView converts one SDK instance to its output shape. +func NewInstanceView(i *verda.Instance) InstanceView { + return InstanceView{ + ID: i.ID, + IP: i.IP, + Status: i.Status, + CreatedAt: nilIfZero(i.CreatedAt), + CPU: i.CPU, + GPU: i.GPU, + GPUMemory: i.GPUMemory, + Memory: i.Memory, + Storage: i.Storage, + Hostname: i.Hostname, + Description: i.Description, + Location: i.Location, + PricePerHour: i.PricePerHour, + IsSpot: i.IsSpot, + InstanceType: i.InstanceType, + Image: i.Image, + OSName: i.OSName, + StartupScriptID: i.StartupScriptID, + SSHKeyIDs: i.SSHKeyIDs, + OSVolumeID: i.OSVolumeID, + JupyterToken: i.JupyterToken, + Contract: i.Contract, + Pricing: i.Pricing, + VolumeIDs: i.VolumeIDs, + } +} + +// NewInstanceViews converts a slice of SDK instances, preserving order. +func NewInstanceViews(instances []verda.Instance) []InstanceView { + views := make([]InstanceView, len(instances)) + for i := range instances { + views[i] = NewInstanceView(&instances[i]) + } + return views +} + +// JobDeploymentShortView is the JSON/YAML shape for one batch-job deployment +// summary. Serverless is a hidden feature; the view exists so the surface does +// not carry the same zero-timestamp defect when it ships. +type JobDeploymentShortView struct { + Name string `json:"name" yaml:"name"` + CreatedAt *time.Time `json:"created_at,omitempty" yaml:"created_at,omitempty"` + Compute *verda.ContainerCompute `json:"compute" yaml:"compute"` +} + +// NewJobDeploymentShortView converts one SDK job deployment summary. +func NewJobDeploymentShortView(j *verda.JobDeploymentShortInfo) JobDeploymentShortView { + return JobDeploymentShortView{ + Name: j.Name, + CreatedAt: nilIfZero(j.CreatedAt), + Compute: j.Compute, + } +} + +// NewJobDeploymentShortViews converts a slice, preserving order. +func NewJobDeploymentShortViews(jobs []verda.JobDeploymentShortInfo) []JobDeploymentShortView { + views := make([]JobDeploymentShortView, len(jobs)) + for i := range jobs { + views[i] = NewJobDeploymentShortView(&jobs[i]) + } + return views +} + // TimeColumn renders a timestamp for table output. An absent value prints as // "-" rather than 0001-01-01, so a human reading the table sees "unknown" // instead of a plausible-looking date. diff --git a/internal/verda-cli/cmd/util/views_test.go b/internal/verda-cli/cmd/util/views_test.go index 8c29d76..820172a 100644 --- a/internal/verda-cli/cmd/util/views_test.go +++ b/internal/verda-cli/cmd/util/views_test.go @@ -16,6 +16,7 @@ package util import ( "bytes" + "reflect" "strings" "testing" "time" @@ -177,3 +178,127 @@ func TestTextColumn(t *testing.T) { t.Errorf("got %q", got) } } + +// jsonKeys returns the json tag names declared on a struct type, ignoring +// options like ",omitempty". +func jsonKeys(t *testing.T, v any) map[string]bool { + t.Helper() + + rt := reflect.TypeOf(v) + keys := make(map[string]bool, rt.NumField()) + for i := range rt.NumField() { + tag := rt.Field(i).Tag.Get("json") + if tag == "" || tag == "-" { + continue + } + keys[strings.Split(tag, ",")[0]] = true + } + return keys +} + +// A view that mirrors an SDK struct field-by-field silently drops any field the +// SDK adds later — and for the agent JSON contract, a silently missing field is +// a broken consumer. This test is the tripwire: it fails when verda.Instance +// grows a field InstanceView does not carry. +func TestInstanceViewCoversSDKFields(t *testing.T) { + t.Parallel() + + sdk := jsonKeys(t, verda.Instance{}) + view := jsonKeys(t, InstanceView{}) + + for key := range sdk { + if !view[key] { + t.Errorf("verda.Instance has json key %q that InstanceView drops — add it to the view", key) + } + } + for key := range view { + if !sdk[key] { + t.Errorf("InstanceView invents json key %q that verda.Instance does not have", key) + } + } +} + +func TestJobDeploymentShortViewCoversSDKFields(t *testing.T) { + t.Parallel() + + sdk := jsonKeys(t, verda.JobDeploymentShortInfo{}) + view := jsonKeys(t, JobDeploymentShortView{}) + + for key := range sdk { + if !view[key] { + t.Errorf("verda.JobDeploymentShortInfo has json key %q that the view drops", key) + } + } + for key := range view { + if !sdk[key] { + t.Errorf("view invents json key %q", key) + } + } +} + +func TestInstanceViewOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + gotJSON, gotYAML := marshalBoth(t, NewInstanceView(&verda.Instance{ID: "inst-1", Hostname: "box"})) + + if strings.Contains(gotJSON, "created_at") { + t.Errorf("zero CreatedAt emitted in JSON: %s", gotJSON) + } + if !strings.Contains(gotJSON, `"hostname": "box"`) { + t.Errorf("hostname lost: %s", gotJSON) + } + if strings.Contains(gotYAML, "0001-01-01") || strings.Contains(gotYAML, "createdat") { + t.Errorf("YAML leaks a zero timestamp or an untagged key:\n%s", gotYAML) + } +} + +func TestInstanceViewKeepsRealCreatedAt(t *testing.T) { + t.Parallel() + + ts := time.Date(2026, 8, 11, 18, 51, 12, 0, time.UTC) + gotJSON, gotYAML := marshalBoth(t, NewInstanceView(&verda.Instance{ID: "inst-1", CreatedAt: ts})) + + if !strings.Contains(gotJSON, `"created_at": "2026-08-11T18:51:12Z"`) { + t.Errorf("real timestamp lost or reformatted: %s", gotJSON) + } + if !strings.Contains(gotYAML, "created_at:") { + t.Errorf("YAML lost created_at:\n%s", gotYAML) + } +} + +func TestInstanceViewsPreserveOrderAndPerRowOmission(t *testing.T) { + t.Parallel() + + ts := time.Date(2026, 8, 11, 18, 51, 12, 0, time.UTC) + views := NewInstanceViews([]verda.Instance{ + {ID: "a"}, + {ID: "b", CreatedAt: ts}, + }) + if len(views) != 2 || views[0].ID != "a" || views[1].ID != "b" { + t.Fatalf("order or length changed: %+v", views) + } + if views[0].CreatedAt != nil { + t.Errorf("row 0 gained a timestamp: %v", views[0].CreatedAt) + } + if views[1].CreatedAt == nil || !views[1].CreatedAt.Equal(ts) { + t.Errorf("row 1 lost its timestamp: %v", views[1].CreatedAt) + } +} + +func TestJobDeploymentShortViewOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + gotJSON, gotYAML := marshalBoth(t, NewJobDeploymentShortView(&verda.JobDeploymentShortInfo{Name: "job-a"})) + if strings.Contains(gotJSON, "created_at") { + t.Errorf("zero CreatedAt emitted in JSON: %s", gotJSON) + } + if strings.Contains(gotYAML, "0001-01-01") { + t.Errorf("zero CreatedAt emitted in YAML:\n%s", gotYAML) + } + + ts := time.Date(2026, 8, 11, 18, 51, 12, 0, time.UTC) + realJSON, _ := marshalBoth(t, NewJobDeploymentShortView(&verda.JobDeploymentShortInfo{Name: "job-b", CreatedAt: ts})) + if !strings.Contains(realJSON, "2026-08-11T18:51:12Z") { + t.Errorf("real timestamp lost: %s", realJSON) + } +} diff --git a/internal/verda-cli/cmd/vm/create.go b/internal/verda-cli/cmd/vm/create.go index 1d57bff..d36d6ad 100644 --- a/internal/verda-cli/cmd/vm/create.go +++ b/internal/verda-cli/cmd/vm/create.go @@ -246,7 +246,7 @@ func runCreate(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStream } // Structured output: emit JSON and return (optionally after waiting). - if wrote, werr := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), instance); wrote { + if wrote, werr := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), cmdutil.NewInstanceView(instance)); wrote { if werr != nil { return werr } diff --git a/internal/verda-cli/cmd/vm/describe.go b/internal/verda-cli/cmd/vm/describe.go index 4e5b6b4..ee0e694 100644 --- a/internal/verda-cli/cmd/vm/describe.go +++ b/internal/verda-cli/cmd/vm/describe.go @@ -84,7 +84,7 @@ func runDescribe(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStre cmdutil.DebugJSON(ioStreams.ErrOut, f.Debug(), "Instance details:", inst) // Structured output. - if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), inst); wrote { + if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), cmdutil.NewInstanceView(inst)); wrote { return err } diff --git a/internal/verda-cli/cmd/vm/list.go b/internal/verda-cli/cmd/vm/list.go index 77ea3ad..d04709b 100644 --- a/internal/verda-cli/cmd/vm/list.go +++ b/internal/verda-cli/cmd/vm/list.go @@ -91,7 +91,7 @@ func runList(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams, cmdutil.DebugJSON(ioStreams.ErrOut, f.Debug(), fmt.Sprintf("API response: %d instance(s):", len(instances)), instances) // Structured output: emit JSON/YAML and return. - if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), instances); wrote { + if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), cmdutil.NewInstanceViews(instances)); wrote { return err } diff --git a/internal/verda-cli/cmd/vm/list_created_at_test.go b/internal/verda-cli/cmd/vm/list_created_at_test.go new file mode 100644 index 0000000..cb8bf7a --- /dev/null +++ b/internal/verda-cli/cmd/vm/list_created_at_test.go @@ -0,0 +1,125 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package vm + +import ( + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// /v1/instances omits created_at, exactly like /v1/ssh-keys — verified against +// the captured staging payload (temp/docs/c1-ondemand-instance.json has no +// time-valued key at all). Emitting Go's zero time here is worse than for keys: +// an age-based reaper acts on running instances. +const instancesBodyNoCreatedAt = `[{"id":"inst-1","hostname":"box-a","status":"running",` + + `"instance_type":"1V100.6V","location":"FIN-01","price_per_hour":1.23},` + + `{"id":"inst-2","hostname":"box-b","status":"offline",` + + `"instance_type":"1V100.6V","location":"FIN-01","price_per_hour":1.23,` + + `"created_at":"2026-08-11T18:51:12Z"}]` + +func runVMListJSON(t *testing.T, body string) string { + t.Helper() + + mux := baseMux() + mux.HandleFunc("GET /instances", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + }) + h := newTestHarness(t, mux) + + root := &cobra.Command{Use: "verda", SilenceUsage: true, SilenceErrors: true} + root.AddCommand(NewCmdList(h.Factory, h.IOStreams)) + root.SetArgs([]string{"list"}) + if err := root.Execute(); err != nil { + t.Fatalf("vm list: %v", err) + } + return h.Stdout.String() +} + +func TestListOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + out := runVMListJSON(t, instancesBodyNoCreatedAt) + + if strings.Contains(out, "0001-01-01") { + t.Errorf("vm list emits a zero timestamp as data:\n%s", out) + } + + var got []map[string]any + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + if len(got) != 2 { + t.Fatalf("len = %d, want 2:\n%s", len(got), out) + } + if _, ok := got[0]["created_at"]; ok { + t.Errorf("undated instance carries created_at: %v", got[0]["created_at"]) + } + if got[1]["created_at"] != "2026-08-11T18:51:12Z" { + t.Errorf("dated instance created_at = %v, want it preserved", got[1]["created_at"]) + } + + // The agent contract must survive the view mapping. + for _, key := range []string{"id", "hostname", "status", "instance_type", "location", "price_per_hour"} { + if _, ok := got[0][key]; !ok { + t.Errorf("field %q dropped by the view: %v", key, got[0]) + } + } + if got[0]["hostname"] != "box-a" || got[0]["id"] != "inst-1" { + t.Errorf("identity fields wrong: %v", got[0]) + } + if got[0]["price_per_hour"] != 1.23 { + t.Errorf("price_per_hour = %v, want 1.23 (number, not string)", got[0]["price_per_hour"]) + } +} + +func TestListEmptyInstances(t *testing.T) { + t.Parallel() + + out := runVMListJSON(t, `[]`) + if strings.Contains(out, "0001-01-01") { + t.Errorf("empty list emitted a timestamp:\n%s", out) + } +} + +func TestDescribeOmitsZeroCreatedAt(t *testing.T) { + t.Parallel() + + mux := baseMux() + mux.HandleFunc("GET /instances/inst-1", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"inst-1","hostname":"box-a","status":"running"}`)) + }) + h := newTestHarness(t, mux) + + root := &cobra.Command{Use: "verda", SilenceUsage: true, SilenceErrors: true} + root.AddCommand(NewCmdDescribe(h.Factory, h.IOStreams)) + root.SetArgs([]string{"describe", "inst-1"}) + if err := root.Execute(); err != nil { + t.Fatalf("vm describe: %v", err) + } + + out := h.Stdout.String() + if strings.Contains(out, "0001-01-01") { + t.Errorf("vm describe emits a zero timestamp as data:\n%s", out) + } + if !strings.Contains(out, "box-a") { + t.Errorf("describe output lost the hostname:\n%s", out) + } +} From 5ce318cfdaf18efa897d3662af5f6e84d9db2652 Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 17:05:58 +0300 Subject: [PATCH 08/18] test(util): assert the view copies every field, not just every key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestInstanceViewCoversSDKFields compares json tag names, which cannot see a field InstanceView declares but NewInstanceView never assigns: it marshals as a zero value and the guard passes — the very defect class these views exist to prevent, and exactly where a hand-mirrored 24-field constructor goes wrong. Fill every verda.Instance field with a distinctive non-zero value by reflection, marshal SDK struct and view, compare the decoded maps key-by-key. Plus a zero-instance check that created_at is the only key dropped, so no other field can silently leave the agent contract. Verified necessary, not redundant: deleting the Hostname assignment from NewInstanceView fails TestInstanceViewCopiesEveryValue with `key "hostname": view has "", SDK has "s21"` while the tag-name guard still passes. Hole identified by session 862d36ab's cross-check of c27e9df; probes adopted as permanent tests and credited in the source. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/util/views_test.go | 111 ++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/internal/verda-cli/cmd/util/views_test.go b/internal/verda-cli/cmd/util/views_test.go index 820172a..fe0edcf 100644 --- a/internal/verda-cli/cmd/util/views_test.go +++ b/internal/verda-cli/cmd/util/views_test.go @@ -16,7 +16,9 @@ package util import ( "bytes" + "encoding/json" "reflect" + "strconv" "strings" "testing" "time" @@ -302,3 +304,112 @@ func TestJobDeploymentShortViewOmitsZeroCreatedAt(t *testing.T) { t.Errorf("real timestamp lost: %s", realJSON) } } + +// fillNonZero recursively sets every settable field to a distinctive non-zero +// value so a copy can be compared field-by-field. time.Time is special-cased: +// its fields are unexported, so recursing into it would find nothing settable. +func fillNonZero(t *testing.T, v reflect.Value, n *int) { + t.Helper() + + *n++ + switch v.Kind() { + case reflect.String: + v.SetString("s" + strconv.Itoa(*n)) + case reflect.Bool: + v.SetBool(true) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + v.SetInt(int64(*n)) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + v.SetUint(uint64(*n)) + case reflect.Float32, reflect.Float64: + v.SetFloat(float64(*n) + 0.25) + case reflect.Pointer: + v.Set(reflect.New(v.Type().Elem())) + fillNonZero(t, v.Elem(), n) + case reflect.Slice: + v.Set(reflect.MakeSlice(v.Type(), 2, 2)) + for i := range 2 { + fillNonZero(t, v.Index(i), n) + } + case reflect.Struct: + if v.Type() == reflect.TypeFor[time.Time]() { + v.Set(reflect.ValueOf(time.Date(2026, 8, 11, 18, 51, 12, 0, time.UTC))) + return + } + for i := range v.NumField() { + if f := v.Field(i); f.CanSet() { + fillNonZero(t, f, n) + } + } + default: + // Maps, channels, funcs, interfaces: absent from these payloads. + } +} + +func marshalToMap(t *testing.T, v any) map[string]any { + t.Helper() + + raw, err := json.Marshal(v) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var m map[string]any + if err := json.Unmarshal(raw, &m); err != nil { + t.Fatalf("unmarshal: %v", err) + } + return m +} + +// The tag-name drift guard cannot see a field InstanceView declares but +// NewInstanceView never assigns: it would marshal as a zero value and still +// pass — the very defect class these views exist to prevent. Fill every SDK +// field with a distinctive value and compare the marshaled maps. +// Credit: hole identified by session 862d36ab's cross-check of c27e9df. +func TestInstanceViewCopiesEveryValue(t *testing.T) { + t.Parallel() + + var inst verda.Instance + n := 0 + fillNonZero(t, reflect.ValueOf(&inst).Elem(), &n) + + sdk := marshalToMap(t, inst) + view := marshalToMap(t, NewInstanceView(&inst)) + + for key, want := range sdk { + got, ok := view[key] + if !ok { + t.Errorf("view dropped key %q", key) + continue + } + if !reflect.DeepEqual(got, want) { + t.Errorf("key %q: view has %#v, SDK has %#v", key, got, want) + } + } + for key := range view { + if _, ok := sdk[key]; !ok { + t.Errorf("view invented key %q", key) + } + } +} + +// On a zero-valued instance the view must drop created_at and nothing else: no +// other field may silently vanish from the agent contract. +func TestEmptyInstanceOnlyDropsCreatedAt(t *testing.T) { + t.Parallel() + + sdk := marshalToMap(t, verda.Instance{}) + view := marshalToMap(t, NewInstanceView(&verda.Instance{})) + + var missing []string + for key := range sdk { + if _, ok := view[key]; !ok { + missing = append(missing, key) + } + } + if len(missing) != 1 || missing[0] != "created_at" { + t.Errorf("view drops %v on a zero instance; want exactly [created_at]", missing) + } + if len(view) != len(sdk)-1 { + t.Errorf("view has %d keys, SDK has %d; want exactly one fewer", len(view), len(sdk)) + } +} From ac0a287005749ac3a69aeafc2f30b42c27de1b2d Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 18:27:13 +0300 Subject: [PATCH 09/18] docs(vm): drop the --description length limit; it does not exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported 100-character cap is not real. Verified live against staging: `vm create --description <101 chars>` was accepted and the full 101-character value came back in the instance payload. My earlier help text stated a limit the API does not enforce, which is worse than saying nothing — it would push users to truncate valid input. Reverts that clause from 03d45e5 and records the measurement inline so nobody re-adds it from the original report. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/vm/create.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/verda-cli/cmd/vm/create.go b/internal/verda-cli/cmd/vm/create.go index d36d6ad..30ea643 100644 --- a/internal/verda-cli/cmd/vm/create.go +++ b/internal/verda-cli/cmd/vm/create.go @@ -172,7 +172,9 @@ func NewCmdCreate(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command flags.StringVar(&opts.Image, "os", "", "OS image slug or an existing detached OS volume ID") flags.StringVar(&opts.Image, "image", "", "Alias of --os") flags.StringVar(&opts.Hostname, "hostname", "", "Hostname for the new VM") - flags.StringVar(&opts.Description, "description", "", "Human-readable description, max 100 characters; defaults to the hostname") + // No length limit stated: the reported 100-char cap does not exist — a + // 101-char description was accepted and stored by staging on 2026-08-12. + flags.StringVar(&opts.Description, "description", "", "Human-readable description; defaults to the hostname") flags.StringSliceVar(&opts.SSHKeyIDs, "ssh-key", nil, "SSH key ID to inject into the instance; repeat the flag for multiple keys") flags.StringSliceVar(&opts.SSHKeyIDs, "ssh-key-id", nil, "Alias of --ssh-key") flags.StringVar(&opts.LocationCode, "location", opts.LocationCode, "Location code, for example FIN-01") From 0b0cd0287c31cd294f78ba2ca41d3125a99b9d90 Mon Sep 17 00:00:00 2001 From: lei Date: Wed, 12 Aug 2026 18:47:42 +0300 Subject: [PATCH 10/18] fix(errors): one error contract for CLI and MCP, and a usable ssh-key 400 F3 exposed a structural problem, not a missing special case. The agent error contract existed twice: cmdutil.AgentError + ClassifyError on the CLI side, and a private argError twin in the MCP server whose renderer only understood its own type. Every API failure therefore reached agents over MCP as a bare string with no code, while docs/agent-errors.md claimed MCP "reuses this contract". Collapse it onto one type. MCP's argument-error constructors now return *cmdutil.AgentError (keeping MCP's "argument" wording), and toolErrorResult renders every failure through ClassifyError. An API 404 now reaches an agent as NOT_FOUND over MCP exactly as on the CLI, and a code added to the classifier appears on both surfaces without touching either renderer. With one funnel, the ssh-key case is a mapping rather than a hook: classifyAPIError recognizes the 400 whose text cannot be shown to a user. POST /instances rejects a request that omits ssh_key_ids with "SSH keys can be an array of UUID's, a single UUID string, null value or not defined" -- while the field was not defined. Verified live on staging with the request body captured via --debug: no ssh_key_ids was sent, so the CLI was right and the server contradicts itself. Users now get SSH_KEY_REQUIRED, exit 2, naming the flag and how to list ids, with the server's wording preserved verbatim in details.api_message. Live: `vm create` without --ssh-key exits 2 with the new envelope; no resource created. Also fixes two tests that passed silently when their errors.As assertion failed. Contract change: MCP tool errors that used to be plain text now carry the envelope. Documented, with the remaining 45 handler call sites that bypass toolErrorResult tracked as follow-up. Co-Authored-By: Claude Opus 5 (1M context) --- docs/agent-errors.md | 31 +++++++- internal/verda-cli/cmd/mcp/server.go | 75 ++++++++++--------- internal/verda-cli/cmd/mcp/server_test.go | 68 +++++++++++++++-- internal/verda-cli/cmd/mcp/tools_vm.go | 4 +- internal/verda-cli/cmd/util/agent_error.go | 32 ++++++++ .../verda-cli/cmd/util/agent_error_test.go | 39 ++++++++++ .../verda-cli/cmd/vm/list_created_at_test.go | 44 +++++++++++ 7 files changed, 248 insertions(+), 45 deletions(-) diff --git a/docs/agent-errors.md b/docs/agent-errors.md index 1ba71dd..4753931 100644 --- a/docs/agent-errors.md +++ b/docs/agent-errors.md @@ -146,6 +146,31 @@ The requested resource does not exist. **Agent action:** Verify the resource ID is correct. List resources to find the right one. +### `SSH_KEY_REQUIRED` + +**Exit code:** 2 + +`POST /instances` requires at least one SSH key and the request carried none. The +API's own text lists an absent value as acceptable and then rejects it, so the +server wording is preserved under `details.api_message` rather than shown as the +message. + +```json +{ + "error": { + "code": "SSH_KEY_REQUIRED", + "message": "the API requires at least one SSH key to create an instance, and this request had none: pass --ssh-key (CLI) or ssh_key_ids (MCP); list ids with \"verda ssh-key list\"", + "details": { + "status": 400, + "api_message": "SSH keys can be an array of UUID's, a single UUID string, null value or not defined" + } + } +} +``` + +**Agent action:** Call `list_ssh_keys` (or `verda ssh-key list`), then retry with +at least one key id. Creating a key first is `add_ssh_key` / `verda ssh-key add`. + ### `INSUFFICIENT_BALANCE` **Exit code:** 6 @@ -260,4 +285,8 @@ Tools exposed by `verda mcp serve` reuse this contract with one transport differ {"error": {"code": "CONFIRMATION_REQUIRED", "message": "action \"delete\" creates billing or destructive changes and requires an explicit confirm: true argument", "details": {"action": "delete"}}} ``` -All other tool failures (API errors, auth, unknown IDs) arrive as plain-text `isError` results. See `internal/verda-cli/cmd/mcp/README.md` for the full tool reference. +**One error type, one classifier.** MCP argument errors are plain `cmdutil.AgentError` values — not a parallel type — and `toolErrorResult` renders *every* failure through `ClassifyError`, the same funnel the CLI uses. So an API 404 reaches an agent as `NOT_FOUND` over MCP exactly as it does on the CLI, and a new code added to the classifier appears on both surfaces without touching either renderer. + +> Changed 2026-08-12: MCP previously carried a private `argError` type, so only argument errors had a code and every other failure degraded to a bare string. Tool errors that used to be plain text now arrive as the envelope. Handlers still returning `mcp.NewToolResultError(err.Error())` directly bypass this — converting the remaining call sites is tracked separately. + +See `internal/verda-cli/cmd/mcp/README.md` for the full tool reference. diff --git a/internal/verda-cli/cmd/mcp/server.go b/internal/verda-cli/cmd/mcp/server.go index 3cc3f91..bdce641 100644 --- a/internal/verda-cli/cmd/mcp/server.go +++ b/internal/verda-cli/cmd/mcp/server.go @@ -17,7 +17,6 @@ package mcp import ( "context" "encoding/json" - "errors" "fmt" "os" "strings" @@ -27,6 +26,8 @@ import ( "github.com/mark3labs/mcp-go/server" pkgversion "github.com/verda-cloud/verda-cli/pkg/version" "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" ) // clientFunc is a function that returns a Verda client on demand. @@ -99,59 +100,63 @@ func jsonResult(data any) (*mcp.CallToolResult, error) { return mcp.NewToolResultText(string(b)), nil } -// argError is a typed argument error carrying the CLI agent-contract code and -// details (docs/agent-errors.md), rendered into MCP tool-error payloads. -type argError struct { - code string - message string - details map[string]any -} - -func (e *argError) Error() string { return e.message } +// Argument-contract errors are plain cmdutil.AgentError values: one error type +// for the whole CLI, so toolErrorResult renders MCP failures through the same +// classifier the CLI uses (docs/agent-errors.md). The wording is MCP's +// ("argument", not "flag"); the codes and details are the shared contract. +// ExitCode is unused over MCP — there is no process to exit — but it costs +// nothing and keeps these values interchangeable with the CLI's. -func missingArgError(name string) *argError { - return &argError{ - code: "MISSING_REQUIRED_FLAGS", - message: fmt.Sprintf("missing required argument %q", name), - details: map[string]any{"missing": []string{name}}, +func missingArgError(name string) *cmdutil.AgentError { + return &cmdutil.AgentError{ + Code: "MISSING_REQUIRED_FLAGS", + Message: fmt.Sprintf("missing required argument %q", name), + Details: map[string]any{"missing": []string{name}}, + ExitCode: cmdutil.ExitBadArgs, } } -func invalidArgError(name, reason string) *argError { - return &argError{ - code: "VALIDATION_ERROR", - message: fmt.Sprintf("invalid value for %s: %s", name, reason), - details: map[string]any{"field": name, "reason": reason}, +func invalidArgError(name, reason string) *cmdutil.AgentError { + return &cmdutil.AgentError{ + Code: "VALIDATION_ERROR", + Message: fmt.Sprintf("invalid value for %s: %s", name, reason), + Details: map[string]any{"field": name, "reason": reason}, + ExitCode: cmdutil.ExitBadArgs, } } // confirmationRequiredError mirrors the CLI's agent-mode CONFIRMATION_REQUIRED // contract: destructive and billing tools refuse to run without confirm=true. -func confirmationRequiredError(action string) *argError { - return &argError{ - code: "CONFIRMATION_REQUIRED", - message: fmt.Sprintf("action %q creates billing or destructive changes and requires an explicit confirm: true argument", action), - details: map[string]any{"action": action}, +func confirmationRequiredError(action string) *cmdutil.AgentError { + return &cmdutil.AgentError{ + Code: "CONFIRMATION_REQUIRED", + Message: fmt.Sprintf("action %q creates billing or destructive changes and requires an explicit confirm: true argument", action), + Details: map[string]any{"action": action}, + ExitCode: cmdutil.ExitBadArgs, } } -// toolErrorResult renders err as an MCP tool-error result. argErrors serialize -// to the agent-contract JSON envelope ({"error": {code, message, details}}) so -// agents can branch on code the same way as with `verda --agent` stderr. +// toolErrorResult renders any error as an MCP tool-error result carrying the +// agent-contract JSON envelope ({"error": {code, message, details}}), so agents +// can branch on code exactly as they do on `verda --agent` stderr. +// +// Every error goes through cmdutil.ClassifyError — the same funnel the CLI uses — +// so an API 404 reaches an agent as NOT_FOUND here too. Before, only MCP's own +// argument errors carried a code and everything else degraded to a bare string. func toolErrorResult(err error) *mcp.CallToolResult { - var ae *argError - if !errors.As(err, &ae) { - return mcp.NewToolResultError(err.Error()) + ae := cmdutil.ClassifyError(err) + if ae == nil { + return mcp.NewToolResultError("unknown error") } b, mErr := json.Marshal(map[string]any{ "error": map[string]any{ - "code": ae.code, - "message": ae.message, - "details": ae.details, + "code": ae.Code, + "message": ae.Message, + "details": ae.Details, }, }) if mErr != nil { - return mcp.NewToolResultError(ae.message) + return mcp.NewToolResultError(ae.Message) } return mcp.NewToolResultError(string(b)) } diff --git a/internal/verda-cli/cmd/mcp/server_test.go b/internal/verda-cli/cmd/mcp/server_test.go index 65418c0..c164126 100644 --- a/internal/verda-cli/cmd/mcp/server_test.go +++ b/internal/verda-cli/cmd/mcp/server_test.go @@ -17,9 +17,13 @@ package mcp import ( "encoding/json" "errors" + "strings" "testing" "github.com/mark3labs/mcp-go/mcp" + "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" ) // resultText extracts the text payload of a single-content tool result. @@ -76,9 +80,15 @@ func TestRequiredString(t *testing.T) { if err == nil { t.Fatal("expected error for missing arg") } - var ae *argError - if errors.As(err, &ae) && ae.code != "MISSING_REQUIRED_FLAGS" { - t.Errorf("code = %q, want MISSING_REQUIRED_FLAGS", ae.code) + // errors.As must succeed — the old form (`As(...) && code != x`) passed + // silently whenever the type assertion failed, which is the case this + // asserts. + var ae *cmdutil.AgentError + if !errors.As(err, &ae) { + t.Fatalf("error is not a *cmdutil.AgentError: %T %v", err, err) + } + if ae.Code != "MISSING_REQUIRED_FLAGS" { + t.Errorf("code = %q, want MISSING_REQUIRED_FLAGS", ae.Code) } a["num"] = float64(7) @@ -86,8 +96,11 @@ func TestRequiredString(t *testing.T) { if err == nil { t.Fatal("expected error for non-string arg") } - if errors.As(err, &ae) && ae.code != "VALIDATION_ERROR" { - t.Errorf("code = %q, want VALIDATION_ERROR", ae.code) + if !errors.As(err, &ae) { + t.Fatalf("error is not a *cmdutil.AgentError: %T %v", err, err) + } + if ae.Code != "VALIDATION_ERROR" { + t.Errorf("code = %q, want VALIDATION_ERROR", ae.Code) } } @@ -220,10 +233,49 @@ func TestToolErrorResultEnvelope(t *testing.T) { t.Errorf("details.action = %v, want create_vm", env.Error.Details["action"]) } - // Non-argError falls back to plain text. + // CONTRACT CHANGE: every error now carries the envelope, not just MCP's own + // argument errors. An agent can branch on code for API failures too, which + // is what docs/agent-errors.md always claimed MCP did. res = toolErrorResult(errors.New("boom")) - if !res.IsError || resultText(t, res) != "boom" { - t.Errorf("plain error = %q, IsError=%v; want boom, true", resultText(t, res), res.IsError) + if !res.IsError { + t.Fatal("expected IsError") + } + if err := json.Unmarshal([]byte(resultText(t, res)), &env); err != nil { + t.Fatalf("plain error did not produce the contract envelope: %v", err) + } + if env.Error.Message != "boom" { + t.Errorf("message = %q, want boom", env.Error.Message) + } + if env.Error.Code == "" { + t.Error("envelope has no code") + } + + // An SDK API error must reach the agent with the mapped code, not a string. + res = toolErrorResult(&verda.APIError{StatusCode: 404, Message: "instance not found"}) + if err := json.Unmarshal([]byte(resultText(t, res)), &env); err != nil { + t.Fatalf("API error did not produce the contract envelope: %v", err) + } + if env.Error.Code != "NOT_FOUND" { + t.Errorf("code = %q, want NOT_FOUND", env.Error.Code) + } + + // The create-time SSH-key 400: the API's self-contradictory text must be + // replaced by something actionable, with the original kept in details. + res = toolErrorResult(&verda.APIError{ + StatusCode: 400, + Message: "SSH keys can be an array of UUID's, a single UUID string, null value or not defined", + }) + if err := json.Unmarshal([]byte(resultText(t, res)), &env); err != nil { + t.Fatalf("ssh-key 400 did not produce the contract envelope: %v", err) + } + if env.Error.Code != "SSH_KEY_REQUIRED" { + t.Errorf("code = %q, want SSH_KEY_REQUIRED", env.Error.Code) + } + if !strings.Contains(env.Error.Message, "ssh_key_ids") { + t.Errorf("message must name the MCP parameter: %q", env.Error.Message) + } + if env.Error.Details["api_message"] == nil { + t.Error("details must keep the verbatim api_message") } } diff --git a/internal/verda-cli/cmd/mcp/tools_vm.go b/internal/verda-cli/cmd/mcp/tools_vm.go index 3b3cf1a..bd4da49 100644 --- a/internal/verda-cli/cmd/mcp/tools_vm.go +++ b/internal/verda-cli/cmd/mcp/tools_vm.go @@ -385,7 +385,9 @@ func (s *Server) handleCreateVM(ctx context.Context, req mcp.CallToolRequest) (* inst, err := client.Instances.Create(ctx, createReq) if err != nil { - return mcp.NewToolResultError(err.Error()), nil + // Contract envelope, not a bare string: a create failure is the one an + // agent most needs to branch on (SSH_KEY_REQUIRED, INSUFFICIENT_BALANCE). + return toolErrorResult(err), nil } if wait { diff --git a/internal/verda-cli/cmd/util/agent_error.go b/internal/verda-cli/cmd/util/agent_error.go index 39e392d..20cb8a2 100644 --- a/internal/verda-cli/cmd/util/agent_error.go +++ b/internal/verda-cli/cmd/util/agent_error.go @@ -237,11 +237,43 @@ func classifyAPIError(apiErr *verda.APIError) *AgentError { Details: map[string]any{"status": apiErr.StatusCode}, ExitCode: ExitInsufficientBal, } + case http.StatusBadRequest: + if ae := sshKeyRequired(apiErr); ae != nil { + return ae + } + return NewAPIError(apiErr.Error(), apiErr.StatusCode) default: return NewAPIError(apiErr.Error(), apiErr.StatusCode) } } +// sshKeyRequired recognizes the one API 400 whose own text cannot be shown to a +// user: POST /instances rejects a request that omits ssh_key_ids with "SSH keys +// can be an array of UUID's, a single UUID string, null value or not defined" — +// while the field *was* not defined. Verified live on staging 2026-08-12 with the +// request body captured via --debug: no ssh_key_ids key was sent. Passing that +// through tells the user their correct input was wrong, in the one wording they +// cannot act on. +// +// Returns nil for any other 400 so the generic API_ERROR path still applies. +// Delete this once the API either accepts an absent value or says what it means. +func sshKeyRequired(apiErr *verda.APIError) *AgentError { + if !strings.Contains(strings.ToLower(apiErr.Message), "ssh key") { + return nil + } + return &AgentError{ + Code: "SSH_KEY_REQUIRED", + Message: "the API requires at least one SSH key to create an instance, and this request had none: " + + "pass --ssh-key (CLI) or ssh_key_ids (MCP); list ids with \"verda ssh-key list\"", + Details: map[string]any{ + "status": apiErr.StatusCode, + // Verbatim: the only record of what the server actually said. + "api_message": apiErr.Message, + }, + ExitCode: ExitBadArgs, + } +} + func isAuthError(msg string) bool { lower := strings.ToLower(msg) return strings.Contains(lower, "no credentials configured") || diff --git a/internal/verda-cli/cmd/util/agent_error_test.go b/internal/verda-cli/cmd/util/agent_error_test.go index 6d2cfbc..2a75887 100644 --- a/internal/verda-cli/cmd/util/agent_error_test.go +++ b/internal/verda-cli/cmd/util/agent_error_test.go @@ -195,3 +195,42 @@ func TestClassifyError_UsageError(t *testing.T) { t.Error("wrapped UsageError lost the VALIDATION_ERROR classification") } } + +// The API rejects a create that omits ssh_key_ids with text that lists an absent +// value as valid (verified live on staging 2026-08-12). Agents and humans get a +// code they can act on instead, and the server's wording survives in details. +func TestClassifySSHKeyRequired(t *testing.T) { + t.Parallel() + + const apiMsg = "SSH keys can be an array of UUID's, a single UUID string, null value or not defined" + ae := ClassifyError(&verda.APIError{StatusCode: 400, Message: apiMsg}) + + if ae.Code != "SSH_KEY_REQUIRED" { + t.Fatalf("code = %q, want SSH_KEY_REQUIRED", ae.Code) + } + if ae.ExitCode != ExitBadArgs { + t.Errorf("exit = %d, want %d (bad input, not an API fault)", ae.ExitCode, ExitBadArgs) + } + if !strings.Contains(ae.Message, "--ssh-key") { + t.Errorf("message must name the CLI flag: %q", ae.Message) + } + if got := ae.Details["api_message"]; got != apiMsg { + t.Errorf("details.api_message = %v, want the verbatim server text", got) + } + if got := ae.Details["status"]; got != 400 { + t.Errorf("details.status = %v, want 400", got) + } +} + +// Any other 400 keeps the generic mapping — the special case must not widen. +func TestClassifyOtherBadRequestStaysAPIError(t *testing.T) { + t.Parallel() + + ae := ClassifyError(&verda.APIError{StatusCode: 400, Message: "hostname already in use"}) + if ae.Code != "API_ERROR" { + t.Errorf("code = %q, want API_ERROR", ae.Code) + } + if ae.ExitCode != ExitAPI { + t.Errorf("exit = %d, want %d", ae.ExitCode, ExitAPI) + } +} diff --git a/internal/verda-cli/cmd/vm/list_created_at_test.go b/internal/verda-cli/cmd/vm/list_created_at_test.go index cb8bf7a..2377037 100644 --- a/internal/verda-cli/cmd/vm/list_created_at_test.go +++ b/internal/verda-cli/cmd/vm/list_created_at_test.go @@ -21,6 +21,8 @@ import ( "testing" "github.com/spf13/cobra" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" ) // /v1/instances omits created_at, exactly like /v1/ssh-keys — verified against @@ -123,3 +125,45 @@ func TestDescribeOmitsZeroCreatedAt(t *testing.T) { t.Errorf("describe output lost the hostname:\n%s", out) } } + +// The API rejects a create that omits ssh_key_ids while its own text says an +// absent value is fine (live staging capture, 2026-08-12: the request body had +// no ssh_key_ids key). The CLI must not relay that wording. +func TestCreateSSHKeyRequiredIsActionable(t *testing.T) { + t.Parallel() + + mux := baseMux() + mux.HandleFunc("POST /instances", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"message":"SSH keys can be an array of UUID's, a single UUID string, null value or not defined"}`)) + }) + h := newTestHarness(t, mux) + + root := &cobra.Command{Use: "verda", SilenceUsage: true, SilenceErrors: true} + root.AddCommand(NewCmdCreate(h.Factory, h.IOStreams)) + root.SetArgs([]string{ + "create", "--kind", "cpu", "--instance-type", "CPU.4V.16G", + "--os", "ubuntu-24.04", "--location", "FIN-00", + "--hostname", "box-a", "--os-volume-size", "50", + }) + + err := root.Execute() + if err == nil { + t.Fatal("expected the create to fail") + } + + ae := cmdutil.ClassifyError(err) + if ae.Code != "SSH_KEY_REQUIRED" { + t.Fatalf("code = %q, want SSH_KEY_REQUIRED (err: %v)", ae.Code, err) + } + if !strings.Contains(ae.Message, "--ssh-key") { + t.Errorf("message must tell the user which flag to pass: %q", ae.Message) + } + if !strings.Contains(ae.Message, "ssh-key list") { + t.Errorf("message should point at how to find ids: %q", ae.Message) + } + if ae.Details["api_message"] == nil { + t.Error("the verbatim server text must survive in details for debugging") + } +} From eff5d867bd09a4e1a14b16156e1c6743280db06f Mon Sep 17 00:00:00 2001 From: lei Date: Thu, 13 Aug 2026 10:16:23 +0300 Subject: [PATCH 11/18] fix(errors): classify the human path too; announce the contract over MCP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-agent mode read err.Error() directly, so the classifier's work only reached --agent callers: a human running `vm create` without --ssh-key still saw the API's self-contradictory "SSH keys can be ... null value or not defined", while an agent got SSH_KEY_REQUIRED plus remediation. Now main classifies once and renders twice — envelope for agents, the same classified message as text for humans. Verified live: the human path prints the actionable message. Human failures stay exit 1. Routing ae.ExitCode there would give scripts 2/3/4/5/6 without --agent, which is the better interface but changes what `$?` means for existing callers; that is a separate decision. MCP now ships server instructions in the initialize response, so every client learns the confirm gate, the error envelope and the accepted vs completed rule before its first tool call, with no client-side change. This is the answer to "how do callers learn the new error shape": they are told, rather than asked to upgrade. Old consumers keep working — the envelope contains the same human message and isError is unchanged. Also tightens the comments added across this branch to the repo's style (design and invariants, one line, no change history — that belongs here), and drops a test comment repeating the claim that /v1/instances omits created_at, which live verification disproved. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/verda/main.go | 12 +++-- internal/verda-cli/cmd/mcp/server.go | 49 ++++++++++++++----- internal/verda-cli/cmd/mcp/server_test.go | 12 ++--- internal/verda-cli/cmd/mcp/tools_vm.go | 4 +- internal/verda-cli/cmd/util/agent_error.go | 16 ++---- .../verda-cli/cmd/util/agent_error_test.go | 5 +- internal/verda-cli/cmd/util/views.go | 10 ++-- internal/verda-cli/cmd/util/views_test.go | 8 ++- internal/verda-cli/cmd/vm/create.go | 3 +- .../verda-cli/cmd/vm/list_created_at_test.go | 11 ++--- 10 files changed, 68 insertions(+), 62 deletions(-) diff --git a/cmd/verda/main.go b/cmd/verda/main.go index d9b8060..3a57450 100644 --- a/cmd/verda/main.go +++ b/cmd/verda/main.go @@ -35,14 +35,16 @@ func main() { // agent-mode branch so a cancel stays silent there too. return } else if err != nil { - // In agent mode, always emit structured JSON errors. + // One classifier, two renderers: JSON envelope for agents, the same + // classified message as plain text for humans. + ae := cmdutil.ClassifyError(err) + if opts.Agent || cmdutil.IsAgentError(err) { - ae := cmdutil.ClassifyError(err) cmdutil.WriteAgentError(os.Stderr, ae) os.Exit(ae.ExitCode) } - // Normal mode: plain text error. - msg := err.Error() + + msg := ae.Message // For auth-related errors, append profile context so the user // knows which profile was used and how to switch. if isAuthRelated(msg) && opts.AuthOptions != nil { @@ -51,6 +53,8 @@ func main() { msg += "\n hint: run 'verda auth use' to switch profile, or 'verda auth show' to check credentials" } fmt.Fprintln(os.Stderr, msg) + // Human failures stay exit 1; ae.ExitCode is agent-mode only, so `$?` + // keeps its meaning for existing callers. os.Exit(1) } } diff --git a/internal/verda-cli/cmd/mcp/server.go b/internal/verda-cli/cmd/mcp/server.go index bdce641..96b8155 100644 --- a/internal/verda-cli/cmd/mcp/server.go +++ b/internal/verda-cli/cmd/mcp/server.go @@ -53,6 +53,33 @@ func NewLazyServer(getClient clientFunc) *Server { return newServer(getClient) } +// serverInstructions carries the confirm gate and the error envelope — the two +// contracts that decide whether an agent spends money correctly. It is prompt +// context in every session, so keep it short. +const serverInstructions = `Verda Cloud: GPU/CPU instances, volumes, SSH keys, object storage. + +CONFIRM GATE — tools that create billing or destructive changes (create_vm, +create_volume, and vm_action with shutdown/force_shutdown/hibernate/delete) +refuse to run unless you pass confirm: true. Show the user the exact target and +its cost first, then retry with confirm. A refused call has no side effects. + +ERRORS — a failed tool returns isError with a JSON text payload: + {"error": {"code": "...", "message": "...", "details": {...}}} +Branch on code, not on message text. Codes you should handle: + CONFIRMATION_REQUIRED - retry with confirm: true after telling the user + MISSING_REQUIRED_FLAGS - details.missing lists the arguments to supply + VALIDATION_ERROR - details.field + details.reason + SSH_KEY_REQUIRED - call list_ssh_keys, retry with ssh_key_ids + AUTH_ERROR - credentials problem; the user must fix them, not you + NOT_FOUND - re-list to find the correct id + INSUFFICIENT_BALANCE - stop and tell the user; do not retry + API_ERROR - upstream failure; details.status has the HTTP status +details.api_message, when present, is the upstream text kept verbatim. + +STATUS HONESTY — create/action tools return status "accepted" unless you pass +wait: true, which polls and returns "completed". Never tell the user a resource +is ready on an "accepted" result.` + func newServer(getClient clientFunc) *Server { s := &Server{getClient: getClient} @@ -60,6 +87,9 @@ func newServer(getClient clientFunc) *Server { s.mcpServer = server.NewMCPServer( "verda-cloud", ver, + // Rides the initialize response: clients learn the contracts before + // their first tool call, with no client-side change. + server.WithInstructions(serverInstructions), ) s.registerDiscoveryTools() @@ -100,12 +130,9 @@ func jsonResult(data any) (*mcp.CallToolResult, error) { return mcp.NewToolResultText(string(b)), nil } -// Argument-contract errors are plain cmdutil.AgentError values: one error type -// for the whole CLI, so toolErrorResult renders MCP failures through the same -// classifier the CLI uses (docs/agent-errors.md). The wording is MCP's -// ("argument", not "flag"); the codes and details are the shared contract. -// ExitCode is unused over MCP — there is no process to exit — but it costs -// nothing and keeps these values interchangeable with the CLI's. +// Argument errors are cmdutil.AgentError values: one error type across both +// surfaces (docs/agent-errors.md). Wording is MCP's ("argument", not "flag"); +// codes and details are the shared contract. ExitCode is inert over MCP. func missingArgError(name string) *cmdutil.AgentError { return &cmdutil.AgentError{ @@ -136,13 +163,9 @@ func confirmationRequiredError(action string) *cmdutil.AgentError { } } -// toolErrorResult renders any error as an MCP tool-error result carrying the -// agent-contract JSON envelope ({"error": {code, message, details}}), so agents -// can branch on code exactly as they do on `verda --agent` stderr. -// -// Every error goes through cmdutil.ClassifyError — the same funnel the CLI uses — -// so an API 404 reaches an agent as NOT_FOUND here too. Before, only MCP's own -// argument errors carried a code and everything else degraded to a bare string. +// toolErrorResult renders any error as the agent-contract envelope +// ({"error": {code, message, details}}) via cmdutil.ClassifyError — the CLI's +// funnel — so a code added there reaches MCP clients without a change here. func toolErrorResult(err error) *mcp.CallToolResult { ae := cmdutil.ClassifyError(err) if ae == nil { diff --git a/internal/verda-cli/cmd/mcp/server_test.go b/internal/verda-cli/cmd/mcp/server_test.go index c164126..0fb4652 100644 --- a/internal/verda-cli/cmd/mcp/server_test.go +++ b/internal/verda-cli/cmd/mcp/server_test.go @@ -80,9 +80,8 @@ func TestRequiredString(t *testing.T) { if err == nil { t.Fatal("expected error for missing arg") } - // errors.As must succeed — the old form (`As(...) && code != x`) passed - // silently whenever the type assertion failed, which is the case this - // asserts. + // errors.As must succeed: `As(...) && code != x` passes silently when the + // type assertion fails, which is the case under test. var ae *cmdutil.AgentError if !errors.As(err, &ae) { t.Fatalf("error is not a *cmdutil.AgentError: %T %v", err, err) @@ -233,9 +232,7 @@ func TestToolErrorResultEnvelope(t *testing.T) { t.Errorf("details.action = %v, want create_vm", env.Error.Details["action"]) } - // CONTRACT CHANGE: every error now carries the envelope, not just MCP's own - // argument errors. An agent can branch on code for API failures too, which - // is what docs/agent-errors.md always claimed MCP did. + // Every error carries the envelope, not only MCP's own argument errors. res = toolErrorResult(errors.New("boom")) if !res.IsError { t.Fatal("expected IsError") @@ -259,8 +256,7 @@ func TestToolErrorResultEnvelope(t *testing.T) { t.Errorf("code = %q, want NOT_FOUND", env.Error.Code) } - // The create-time SSH-key 400: the API's self-contradictory text must be - // replaced by something actionable, with the original kept in details. + // Create-time SSH-key 400: actionable message, upstream text in details. res = toolErrorResult(&verda.APIError{ StatusCode: 400, Message: "SSH keys can be an array of UUID's, a single UUID string, null value or not defined", diff --git a/internal/verda-cli/cmd/mcp/tools_vm.go b/internal/verda-cli/cmd/mcp/tools_vm.go index bd4da49..40ca2f4 100644 --- a/internal/verda-cli/cmd/mcp/tools_vm.go +++ b/internal/verda-cli/cmd/mcp/tools_vm.go @@ -385,8 +385,8 @@ func (s *Server) handleCreateVM(ctx context.Context, req mcp.CallToolRequest) (* inst, err := client.Instances.Create(ctx, createReq) if err != nil { - // Contract envelope, not a bare string: a create failure is the one an - // agent most needs to branch on (SSH_KEY_REQUIRED, INSUFFICIENT_BALANCE). + // Envelope, not a bare string: agents branch on create failures + // (SSH_KEY_REQUIRED, INSUFFICIENT_BALANCE). return toolErrorResult(err), nil } diff --git a/internal/verda-cli/cmd/util/agent_error.go b/internal/verda-cli/cmd/util/agent_error.go index 20cb8a2..b92fe13 100644 --- a/internal/verda-cli/cmd/util/agent_error.go +++ b/internal/verda-cli/cmd/util/agent_error.go @@ -247,16 +247,10 @@ func classifyAPIError(apiErr *verda.APIError) *AgentError { } } -// sshKeyRequired recognizes the one API 400 whose own text cannot be shown to a -// user: POST /instances rejects a request that omits ssh_key_ids with "SSH keys -// can be an array of UUID's, a single UUID string, null value or not defined" — -// while the field *was* not defined. Verified live on staging 2026-08-12 with the -// request body captured via --debug: no ssh_key_ids key was sent. Passing that -// through tells the user their correct input was wrong, in the one wording they -// cannot act on. -// -// Returns nil for any other 400 so the generic API_ERROR path still applies. -// Delete this once the API either accepts an absent value or says what it means. +// sshKeyRequired maps the create-time 400 whose upstream text contradicts +// itself: POST /instances rejects an absent ssh_key_ids while listing "not +// defined" as valid, so the wording is unactionable and moves to details. +// nil for any other 400. Delete when the API accepts an absent value. func sshKeyRequired(apiErr *verda.APIError) *AgentError { if !strings.Contains(strings.ToLower(apiErr.Message), "ssh key") { return nil @@ -267,7 +261,7 @@ func sshKeyRequired(apiErr *verda.APIError) *AgentError { "pass --ssh-key (CLI) or ssh_key_ids (MCP); list ids with \"verda ssh-key list\"", Details: map[string]any{ "status": apiErr.StatusCode, - // Verbatim: the only record of what the server actually said. + // Upstream text, verbatim, for debugging. "api_message": apiErr.Message, }, ExitCode: ExitBadArgs, diff --git a/internal/verda-cli/cmd/util/agent_error_test.go b/internal/verda-cli/cmd/util/agent_error_test.go index 2a75887..291ee75 100644 --- a/internal/verda-cli/cmd/util/agent_error_test.go +++ b/internal/verda-cli/cmd/util/agent_error_test.go @@ -196,9 +196,8 @@ func TestClassifyError_UsageError(t *testing.T) { } } -// The API rejects a create that omits ssh_key_ids with text that lists an absent -// value as valid (verified live on staging 2026-08-12). Agents and humans get a -// code they can act on instead, and the server's wording survives in details. +// Unactionable upstream wording becomes a code both surfaces can act on, with +// the original preserved in details. func TestClassifySSHKeyRequired(t *testing.T) { t.Parallel() diff --git a/internal/verda-cli/cmd/util/views.go b/internal/verda-cli/cmd/util/views.go index 0c29949..353b024 100644 --- a/internal/verda-cli/cmd/util/views.go +++ b/internal/verda-cli/cmd/util/views.go @@ -96,13 +96,9 @@ func NewStartupScriptViews(scripts []verda.StartupScript) []StartupScriptView { return views } -// InstanceView is the JSON/YAML shape for one instance. Every field of -// verda.Instance is mirrored with its own json key — TestInstanceViewCoversSDKFields -// fails if the SDK grows a field this view would silently drop. -// -// Nested types (CPU, GPU, …) are reused from the SDK: they carry no zero-time -// field, so there is nothing to omit, and re-declaring them would be four more -// structs to keep in sync. +// InstanceView is the JSON/YAML shape for one instance, mirroring every +// verda.Instance field; TestInstanceViewCoversSDKFields guards against drift. +// Nested types are reused from the SDK — none carries a zero-time field. type InstanceView struct { ID string `json:"id" yaml:"id"` IP *string `json:"ip" yaml:"ip"` diff --git a/internal/verda-cli/cmd/util/views_test.go b/internal/verda-cli/cmd/util/views_test.go index fe0edcf..fc9a350 100644 --- a/internal/verda-cli/cmd/util/views_test.go +++ b/internal/verda-cli/cmd/util/views_test.go @@ -360,11 +360,9 @@ func marshalToMap(t *testing.T, v any) map[string]any { return m } -// The tag-name drift guard cannot see a field InstanceView declares but -// NewInstanceView never assigns: it would marshal as a zero value and still -// pass — the very defect class these views exist to prevent. Fill every SDK -// field with a distinctive value and compare the marshaled maps. -// Credit: hole identified by session 862d36ab's cross-check of c27e9df. +// A tag-name guard cannot see a field the view declares but the constructor +// never assigns — it marshals as a zero value and passes. Fill every SDK field +// with a distinctive value and compare the marshaled maps. func TestInstanceViewCopiesEveryValue(t *testing.T) { t.Parallel() diff --git a/internal/verda-cli/cmd/vm/create.go b/internal/verda-cli/cmd/vm/create.go index 30ea643..6345ff3 100644 --- a/internal/verda-cli/cmd/vm/create.go +++ b/internal/verda-cli/cmd/vm/create.go @@ -172,8 +172,7 @@ func NewCmdCreate(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command flags.StringVar(&opts.Image, "os", "", "OS image slug or an existing detached OS volume ID") flags.StringVar(&opts.Image, "image", "", "Alias of --os") flags.StringVar(&opts.Hostname, "hostname", "", "Hostname for the new VM") - // No length limit stated: the reported 100-char cap does not exist — a - // 101-char description was accepted and stored by staging on 2026-08-12. + // No length limit documented: the API enforces none at 101 characters. flags.StringVar(&opts.Description, "description", "", "Human-readable description; defaults to the hostname") flags.StringSliceVar(&opts.SSHKeyIDs, "ssh-key", nil, "SSH key ID to inject into the instance; repeat the flag for multiple keys") flags.StringSliceVar(&opts.SSHKeyIDs, "ssh-key-id", nil, "Alias of --ssh-key") diff --git a/internal/verda-cli/cmd/vm/list_created_at_test.go b/internal/verda-cli/cmd/vm/list_created_at_test.go index 2377037..0d3b72c 100644 --- a/internal/verda-cli/cmd/vm/list_created_at_test.go +++ b/internal/verda-cli/cmd/vm/list_created_at_test.go @@ -25,10 +25,9 @@ import ( cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" ) -// /v1/instances omits created_at, exactly like /v1/ssh-keys — verified against -// the captured staging payload (temp/docs/c1-ondemand-instance.json has no -// time-valued key at all). Emitting Go's zero time here is worse than for keys: -// an age-based reaper acts on running instances. +// verda.Instance.CreatedAt has no omitempty, so an absent created_at would +// marshal as 0001-01-01 — a plausible date an age-based reaper acts on. +// /v1/instances populates the field today; the view removes the trap either way. const instancesBodyNoCreatedAt = `[{"id":"inst-1","hostname":"box-a","status":"running",` + `"instance_type":"1V100.6V","location":"FIN-01","price_per_hour":1.23},` + `{"id":"inst-2","hostname":"box-b","status":"offline",` + @@ -126,9 +125,7 @@ func TestDescribeOmitsZeroCreatedAt(t *testing.T) { } } -// The API rejects a create that omits ssh_key_ids while its own text says an -// absent value is fine (live staging capture, 2026-08-12: the request body had -// no ssh_key_ids key). The CLI must not relay that wording. +// The upstream 400 contradicts itself; the CLI must not relay that wording. func TestCreateSSHKeyRequiredIsActionable(t *testing.T) { t.Parallel() From d789c326fc41f55ee4efbd76c2d177b98c15833c Mon Sep 17 00:00:00 2001 From: lei Date: Thu, 13 Aug 2026 10:24:41 +0300 Subject: [PATCH 12/18] fix(tui): don't start the pager when output is not a terminal Pager launched an alt-screen Bubble Tea program regardless of destination, so `verda volume trash | head` blocked forever waiting for keys nobody could send. terminalHeight falls back to 24 for a pipe, which makes the trap data-dependent: a short trash list took the print-through path and looked fine, while a full one hung. Automation without --agent was the only victim, since agent mode leaves Status nil and never reaches the pager. Guard on rendersToTerminal and print through, matching Spinner and progress, which already do exactly this. volume trash is the only caller today, and the wizard docs advertise Pager to future ones. Test fails before by timing out after 5s and passes instantly after; live, the piped repro now returns the listing immediately. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/tui/bubbletea/pager.go | 8 +++++ pkg/tui/bubbletea/pager_test.go | 56 +++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 pkg/tui/bubbletea/pager_test.go diff --git a/pkg/tui/bubbletea/pager.go b/pkg/tui/bubbletea/pager.go index 1842381..aa53f9d 100644 --- a/pkg/tui/bubbletea/pager.go +++ b/pkg/tui/bubbletea/pager.go @@ -131,6 +131,14 @@ func (m pagerModel) View() tea.View { func (p *Prompter) Pager(ctx context.Context, content string, opts ...tui.PagerOption) error { cfg := tui.ResolvePagerConfig(opts) + // Paging needs a terminal to page in; elsewhere the content is just data. + // Without this, terminalHeight's 24-line fallback sends anything longer into + // an alt-screen program that waits forever for keys nobody can send. + if !rendersToTerminal(p.out) { + _, err := fmt.Fprint(p.dataOut, content) + return err + } + // Auto-detect: if content fits in terminal, just print it. The // print-through path is data, so it goes to dataOut (house rule). lines := strings.Count(content, "\n") + 1 diff --git a/pkg/tui/bubbletea/pager_test.go b/pkg/tui/bubbletea/pager_test.go new file mode 100644 index 0000000..4b09935 --- /dev/null +++ b/pkg/tui/bubbletea/pager_test.go @@ -0,0 +1,56 @@ +package bubbletea + +import ( + "bytes" + "context" + "strings" + "testing" + "time" + + "github.com/verda-cloud/verda-cli/pkg/tui" +) + +// Pager must not start a Bubble Tea program when its output is not a terminal: +// terminalHeight falls back to 24 there, so any content over ~22 lines would +// launch an alt-screen program against a pipe and block on input forever. +func TestPagerWithoutTerminalPrintsInsteadOfBlocking(t *testing.T) { + t.Parallel() + + content := strings.Repeat("line\n", 200) + var dataOut, uiOut bytes.Buffer + p := New(WithIO(tui.IO{Out: &dataOut, ErrOut: &uiOut, In: strings.NewReader("")})) + + done := make(chan error, 1) + go func() { done <- p.Pager(context.Background(), content, tui.WithPagerTitle("Trash")) }() + + select { + case err := <-done: + if err != nil { + t.Fatalf("Pager: %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("Pager blocked on a non-terminal writer; it must print through instead") + } + + if dataOut.String() != content { + t.Errorf("content not written to data out: got %d bytes, want %d", dataOut.Len(), len(content)) + } + if uiOut.Len() != 0 { + t.Errorf("interactive stream should stay empty, got %q", uiOut.String()) + } +} + +// Short content already took the print-through path; keep that behavior pinned. +func TestPagerShortContentPrintsThrough(t *testing.T) { + t.Parallel() + + var dataOut, uiOut bytes.Buffer + p := New(WithIO(tui.IO{Out: &dataOut, ErrOut: &uiOut, In: strings.NewReader("")})) + + if err := p.Pager(context.Background(), "one\ntwo\n"); err != nil { + t.Fatalf("Pager: %v", err) + } + if dataOut.String() != "one\ntwo\n" { + t.Errorf("got %q", dataOut.String()) + } +} From 54932dbfb0e2ca74997b0085460ed2ce2a9f798a Mon Sep 17 00:00:00 2001 From: lei Date: Thu, 13 Aug 2026 10:40:36 +0300 Subject: [PATCH 13/18] fix(volume): output contract for the volume package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit volume trash never consulted f.OutputFormat() or f.AgentMode(), so `--agent -o json` returned an ANSI-styled table: 40 escape-carrying lines into a redirected stdout. Checking the package turned up the same two defects spread wider than the reported finding. Zero-timestamp class, as in be842d4 and c27e9df: Volume.CreatedAt, VolumeInTrash.CreatedAt and DeletedAt carry no omitempty, and four sites marshal those types raw (list, describe, create, and trash which had no structured branch at all). DeletedAt is the one that matters most — it drives the 96-hour recovery countdown, so a fabricated date misreports how long a volume can still be restored. trash.go already guarded the expiry calculation with !DeletedAt.IsZero() while formatting the date unconditionally two lines above. Add VolumeView and VolumeInTrashView with both guards that earned their keep on InstanceView: json-tag drift and value-copy by reflection. Wire all four sites, render an absent timestamp as "-" via TimeColumn, and gate styling on the expression already used in cp.go, container_list.go and vm/list.go rather than inventing a second one. renderVolumeSummary gets the same gate, since describe and create share it. First trash_test.go in the package. All four tests fail under mutation (structured branch removed, styling forced on). Live: `volume trash --agent -o json` is valid JSON with 0 ANSI lines, table mode redirected is also 0, both exit 0. Closes F2 (order 007). volume trash has no restore path in the CLI or the SDK, so being read by a human or a script is its only purpose. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/util/views.go | 119 +++++++++++++++ internal/verda-cli/cmd/util/views_test.go | 98 +++++++++++++ internal/verda-cli/cmd/volume/create.go | 2 +- internal/verda-cli/cmd/volume/describe.go | 2 +- internal/verda-cli/cmd/volume/list.go | 2 +- internal/verda-cli/cmd/volume/trash.go | 22 ++- internal/verda-cli/cmd/volume/trash_test.go | 155 ++++++++++++++++++++ internal/verda-cli/cmd/volume/view.go | 18 ++- 8 files changed, 408 insertions(+), 10 deletions(-) create mode 100644 internal/verda-cli/cmd/volume/trash_test.go diff --git a/internal/verda-cli/cmd/util/views.go b/internal/verda-cli/cmd/util/views.go index 353b024..41c5899 100644 --- a/internal/verda-cli/cmd/util/views.go +++ b/internal/verda-cli/cmd/util/views.go @@ -165,6 +165,125 @@ func NewInstanceViews(instances []verda.Instance) []InstanceView { return views } +// VolumeView is the JSON/YAML shape for one volume, mirroring every +// verda.Volume field; TestVolumeViewCoversSDKFields guards against drift. +type VolumeView struct { + ID string `json:"id" yaml:"id"` + Name string `json:"name" yaml:"name"` + Size int `json:"size" yaml:"size"` + Type string `json:"type" yaml:"type"` + Status string `json:"status" yaml:"status"` + CreatedAt *time.Time `json:"created_at,omitempty" yaml:"created_at,omitempty"` + InstanceID *string `json:"instance_id" yaml:"instance_id"` + Instances []verda.VolumeAttachedInstance `json:"instances" yaml:"instances"` + Location string `json:"location" yaml:"location"` + Contract string `json:"contract,omitempty" yaml:"contract,omitempty"` + IsOSVolume bool `json:"is_os_volume" yaml:"is_os_volume"` + Target *string `json:"target" yaml:"target"` + SSHKeyIDs []string `json:"ssh_key_ids" yaml:"ssh_key_ids"` + PseudoPath *string `json:"pseudo_path" yaml:"pseudo_path"` + CreateDirectoryCommand *string `json:"create_directory_command" yaml:"create_directory_command"` + MountCommand *string `json:"mount_command" yaml:"mount_command"` + FilesystemToFstabCommand *string `json:"filesystem_to_fstab_command" yaml:"filesystem_to_fstab_command"` + BaseHourlyCost float64 `json:"base_hourly_cost" yaml:"base_hourly_cost"` + MonthlyPrice float64 `json:"monthly_price" yaml:"monthly_price"` + Currency string `json:"currency" yaml:"currency"` + LongTerm *verda.VolumeLongTerm `json:"long_term" yaml:"long_term"` +} + +// NewVolumeView converts one SDK volume to its output shape. +func NewVolumeView(v *verda.Volume) VolumeView { + return VolumeView{ + ID: v.ID, + Name: v.Name, + Size: v.Size, + Type: v.Type, + Status: v.Status, + CreatedAt: nilIfZero(v.CreatedAt), + InstanceID: v.InstanceID, + Instances: v.Instances, + Location: v.Location, + Contract: v.Contract, + IsOSVolume: v.IsOSVolume, + Target: v.Target, + SSHKeyIDs: v.SSHKeyIDs, + PseudoPath: v.PseudoPath, + CreateDirectoryCommand: v.CreateDirectoryCommand, + MountCommand: v.MountCommand, + FilesystemToFstabCommand: v.FilesystemToFstabCommand, + BaseHourlyCost: v.BaseHourlyCost, + MonthlyPrice: v.MonthlyPrice, + Currency: v.Currency, + LongTerm: v.LongTerm, + } +} + +// NewVolumeViews converts a slice of SDK volumes, preserving order. +func NewVolumeViews(volumes []verda.Volume) []VolumeView { + views := make([]VolumeView, len(volumes)) + for i := range volumes { + views[i] = NewVolumeView(&volumes[i]) + } + return views +} + +// VolumeInTrashView is the JSON/YAML shape for one trashed volume. Both +// timestamps are optional: DeletedAt drives the 96-hour recovery countdown, so a +// fabricated date here would misreport how long a volume can still be restored. +type VolumeInTrashView struct { + ID string `json:"id" yaml:"id"` + Name string `json:"name" yaml:"name"` + Size int `json:"size" yaml:"size"` + Type string `json:"type" yaml:"type"` + Status string `json:"status" yaml:"status"` + CreatedAt *time.Time `json:"created_at,omitempty" yaml:"created_at,omitempty"` + DeletedAt *time.Time `json:"deleted_at,omitempty" yaml:"deleted_at,omitempty"` + InstanceID *string `json:"instance_id" yaml:"instance_id"` + Instances []verda.VolumeAttachedInstance `json:"instances" yaml:"instances"` + Location string `json:"location" yaml:"location"` + Contract string `json:"contract" yaml:"contract"` + IsOSVolume bool `json:"is_os_volume" yaml:"is_os_volume"` + Target *string `json:"target" yaml:"target"` + SSHKeyIDs []string `json:"ssh_key_ids" yaml:"ssh_key_ids"` + BaseHourlyCost float64 `json:"base_hourly_cost" yaml:"base_hourly_cost"` + MonthlyPrice float64 `json:"monthly_price" yaml:"monthly_price"` + Currency string `json:"currency" yaml:"currency"` + IsPermanentlyDeleted bool `json:"is_permanently_deleted" yaml:"is_permanently_deleted"` +} + +// NewVolumeInTrashView converts one SDK trashed volume to its output shape. +func NewVolumeInTrashView(v *verda.VolumeInTrash) VolumeInTrashView { + return VolumeInTrashView{ + ID: v.ID, + Name: v.Name, + Size: v.Size, + Type: v.Type, + Status: v.Status, + CreatedAt: nilIfZero(v.CreatedAt), + DeletedAt: nilIfZero(v.DeletedAt), + InstanceID: v.InstanceID, + Instances: v.Instances, + Location: v.Location, + Contract: v.Contract, + IsOSVolume: v.IsOSVolume, + Target: v.Target, + SSHKeyIDs: v.SSHKeyIDs, + BaseHourlyCost: v.BaseHourlyCost, + MonthlyPrice: v.MonthlyPrice, + Currency: v.Currency, + IsPermanentlyDeleted: v.IsPermanentlyDeleted, + } +} + +// NewVolumeInTrashViews converts a slice, preserving order. +func NewVolumeInTrashViews(volumes []verda.VolumeInTrash) []VolumeInTrashView { + views := make([]VolumeInTrashView, len(volumes)) + for i := range volumes { + views[i] = NewVolumeInTrashView(&volumes[i]) + } + return views +} + // JobDeploymentShortView is the JSON/YAML shape for one batch-job deployment // summary. Serverless is a hidden feature; the view exists so the surface does // not carry the same zero-timestamp defect when it ships. diff --git a/internal/verda-cli/cmd/util/views_test.go b/internal/verda-cli/cmd/util/views_test.go index fc9a350..cee6331 100644 --- a/internal/verda-cli/cmd/util/views_test.go +++ b/internal/verda-cli/cmd/util/views_test.go @@ -411,3 +411,101 @@ func TestEmptyInstanceOnlyDropsCreatedAt(t *testing.T) { t.Errorf("view has %d keys, SDK has %d; want exactly one fewer", len(view), len(sdk)) } } + +func TestVolumeViewCoversSDKFields(t *testing.T) { + t.Parallel() + + sdk := jsonKeys(t, verda.Volume{}) + view := jsonKeys(t, VolumeView{}) + for key := range sdk { + if !view[key] { + t.Errorf("verda.Volume has json key %q that VolumeView drops", key) + } + } + for key := range view { + if !sdk[key] { + t.Errorf("VolumeView invents json key %q", key) + } + } +} + +func TestVolumeInTrashViewCoversSDKFields(t *testing.T) { + t.Parallel() + + sdk := jsonKeys(t, verda.VolumeInTrash{}) + view := jsonKeys(t, VolumeInTrashView{}) + for key := range sdk { + if !view[key] { + t.Errorf("verda.VolumeInTrash has json key %q that the view drops", key) + } + } + for key := range view { + if !sdk[key] { + t.Errorf("VolumeInTrashView invents json key %q", key) + } + } +} + +func TestVolumeViewCopiesEveryValue(t *testing.T) { + t.Parallel() + + var vol verda.Volume + n := 0 + fillNonZero(t, reflect.ValueOf(&vol).Elem(), &n) + + sdk := marshalToMap(t, vol) + view := marshalToMap(t, NewVolumeView(&vol)) + for key, want := range sdk { + got, ok := view[key] + if !ok { + t.Errorf("view dropped key %q", key) + continue + } + if !reflect.DeepEqual(got, want) { + t.Errorf("key %q: view has %#v, SDK has %#v", key, got, want) + } + } +} + +func TestVolumeInTrashViewCopiesEveryValue(t *testing.T) { + t.Parallel() + + var vol verda.VolumeInTrash + n := 0 + fillNonZero(t, reflect.ValueOf(&vol).Elem(), &n) + + sdk := marshalToMap(t, vol) + view := marshalToMap(t, NewVolumeInTrashView(&vol)) + for key, want := range sdk { + got, ok := view[key] + if !ok { + t.Errorf("view dropped key %q", key) + continue + } + if !reflect.DeepEqual(got, want) { + t.Errorf("key %q: view has %#v, SDK has %#v", key, got, want) + } + } +} + +// A trashed volume with no deleted_at must not gain one: that field drives the +// 96-hour recovery countdown. +func TestVolumeInTrashViewOmitsZeroTimes(t *testing.T) { + t.Parallel() + + gotJSON, gotYAML := marshalBoth(t, NewVolumeInTrashView(&verda.VolumeInTrash{ID: "vol-1", Name: "orphan"})) + for _, out := range []string{gotJSON, gotYAML} { + if strings.Contains(out, "0001-01-01") { + t.Errorf("zero timestamp emitted:\n%s", out) + } + if strings.Contains(out, "deleted_at") || strings.Contains(out, "created_at") { + t.Errorf("absent timestamps present as keys:\n%s", out) + } + } + + ts := time.Date(2026, 8, 11, 18, 51, 12, 0, time.UTC) + realJSON, _ := marshalBoth(t, NewVolumeInTrashView(&verda.VolumeInTrash{ID: "vol-2", DeletedAt: ts})) + if !strings.Contains(realJSON, `"deleted_at": "2026-08-11T18:51:12Z"`) { + t.Errorf("real deleted_at lost: %s", realJSON) + } +} diff --git a/internal/verda-cli/cmd/volume/create.go b/internal/verda-cli/cmd/volume/create.go index b77f94a..c42b98c 100644 --- a/internal/verda-cli/cmd/volume/create.go +++ b/internal/verda-cli/cmd/volume/create.go @@ -242,7 +242,7 @@ func runCreate(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStream return err } if vol != nil { - if wrote, werr := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), vol); wrote { + if wrote, werr := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), cmdutil.NewVolumeView(vol)); wrote { return werr } } diff --git a/internal/verda-cli/cmd/volume/describe.go b/internal/verda-cli/cmd/volume/describe.go index 4cfd965..d2d67b0 100644 --- a/internal/verda-cli/cmd/volume/describe.go +++ b/internal/verda-cli/cmd/volume/describe.go @@ -86,7 +86,7 @@ func runDescribe(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStre cmdutil.DebugJSON(ioStreams.ErrOut, f.Debug(), "Volume details:", vol) // Structured output. - if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), vol); wrote { + if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), cmdutil.NewVolumeView(vol)); wrote { return err } diff --git a/internal/verda-cli/cmd/volume/list.go b/internal/verda-cli/cmd/volume/list.go index b1bff50..e92bcfc 100644 --- a/internal/verda-cli/cmd/volume/list.go +++ b/internal/verda-cli/cmd/volume/list.go @@ -86,7 +86,7 @@ func runList(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams, cmdutil.DebugJSON(ioStreams.ErrOut, f.Debug(), fmt.Sprintf("API response: %d volume(s):", len(volumes)), volumes) // Structured output: emit JSON/YAML and return. - if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), volumes); wrote { + if wrote, err := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), cmdutil.NewVolumeViews(volumes)); wrote { return err } diff --git a/internal/verda-cli/cmd/volume/trash.go b/internal/verda-cli/cmd/volume/trash.go index c6132dd..0cd57f5 100644 --- a/internal/verda-cli/cmd/volume/trash.go +++ b/internal/verda-cli/cmd/volume/trash.go @@ -20,9 +20,9 @@ import ( "strings" "time" - "charm.land/lipgloss/v2" "github.com/spf13/cobra" "github.com/verda-cloud/verda-cli/pkg/tui" + "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" ) @@ -69,14 +69,17 @@ func runTrash(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams cmdutil.DebugJSON(ioStreams.ErrOut, f.Debug(), fmt.Sprintf("API response: %d trashed volume(s):", len(volumes)), volumes) + if wrote, werr := cmdutil.WriteStructured(ioStreams.Out, f.OutputFormat(), cmdutil.NewVolumeInTrashViews(volumes)); wrote { + return werr + } + if len(volumes) == 0 { _, _ = fmt.Fprintln(ioStreams.Out, "Trash is empty.") return nil } - dim := lipgloss.NewStyle().Foreground(lipgloss.Color("8")) - bold := lipgloss.NewStyle().Bold(true) - warnStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("1")) + // Table mode by construction here; styling still depends on the destination. + dim, bold, warnStyle := trashStyles(cmdutil.IsStdoutTerminal() && !f.AgentMode()) var b strings.Builder _, _ = fmt.Fprintf(&b, " %d volume(s) in trash\n\n", len(volumes)) @@ -101,7 +104,7 @@ func runTrash(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams if v.MonthlyPrice > 0 { _, _ = fmt.Fprintf(&b, " %s $%.2f/mo (%s)\n", dim.Render("Price: "), v.MonthlyPrice, v.Currency) } - _, _ = fmt.Fprintf(&b, " %s %s\n", dim.Render("Deleted: "), v.DeletedAt.Format("2 Jan 2006, 15:04")) + _, _ = fmt.Fprintf(&b, " %s %s\n", dim.Render("Deleted: "), cmdutil.TimeColumn(deletedAt(v), "2 Jan 2006, 15:04")) if !v.IsPermanentlyDeleted && !v.DeletedAt.IsZero() { expiresAt := v.DeletedAt.Add(96 * time.Hour) @@ -121,6 +124,15 @@ func runTrash(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams return nil } +// deletedAt keeps the table honest about an absent timestamp: an unset value +// prints "-" rather than 1 Jan 0001, matching the JSON view's omission. +func deletedAt(v *verda.VolumeInTrash) *time.Time { + if v.DeletedAt.IsZero() { + return nil + } + return &v.DeletedAt +} + func formatDuration(d time.Duration) string { h := int(d.Hours()) if h >= 24 { diff --git a/internal/verda-cli/cmd/volume/trash_test.go b/internal/verda-cli/cmd/volume/trash_test.go new file mode 100644 index 0000000..069d8da --- /dev/null +++ b/internal/verda-cli/cmd/volume/trash_test.go @@ -0,0 +1,155 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package volume + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" +) + +const trashBody = `[{"id":"vol-1","name":"box-a-os","size":50,"type":"NVMe_Shared",` + + `"location":"FIN-00","contract":"PAY_AS_YOU_GO","is_os_volume":true,` + + `"monthly_price":10,"currency":"usd","deleted_at":"2026-08-11T18:51:12Z"},` + + `{"id":"vol-2","name":"undated","size":20,"type":"NVMe_Shared",` + + `"location":"FIN-00","contract":"PAY_AS_YOU_GO","is_os_volume":false}]` + +func runTrashCmd(t *testing.T, body, format string, agent bool) string { + t.Helper() + + mux := http.NewServeMux() + mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "access_token": "test-token", + "token_type": "Bearer", + }) + }) + mux.HandleFunc("GET /volumes/trash", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + client, err := verda.NewClient( + verda.WithBaseURL(srv.URL), + verda.WithClientID("test-id"), + verda.WithClientSecret("test-secret"), + ) + if err != nil { + t.Fatalf("creating client: %v", err) + } + + var out bytes.Buffer + f := &cmdutil.TestFactory{ + ClientOverride: client, + OutputFormatOverride: format, + AgentModeOverride: agent, + } + cmd := NewCmdTrash(f, cmdutil.IOStreams{Out: &out, ErrOut: &bytes.Buffer{}}) + cmd.SetArgs(nil) + cmd.SetContext(context.Background()) + if err := cmd.Execute(); err != nil { + t.Fatalf("volume trash: %v", err) + } + return out.String() +} + +// `trash -o json` used to print an ANSI table: the command never consulted the +// output format at all. +func TestTrashHonorsJSONOutput(t *testing.T) { + t.Parallel() + + got := runTrashCmd(t, trashBody, "json", true) + + if strings.ContainsRune(got, '\033') { + t.Errorf("JSON output carries ANSI escapes:\n%q", got) + } + var rows []map[string]any + if err := json.Unmarshal([]byte(got), &rows); err != nil { + t.Fatalf("not valid JSON: %v\n%s", err, got) + } + if len(rows) != 2 { + t.Fatalf("len = %d, want 2", len(rows)) + } + if rows[0]["deleted_at"] != "2026-08-11T18:51:12Z" { + t.Errorf("deleted_at = %v, want it preserved", rows[0]["deleted_at"]) + } + if _, ok := rows[1]["deleted_at"]; ok { + t.Errorf("undated volume carries deleted_at: %v", rows[1]) + } + if _, ok := rows[1]["created_at"]; ok { + t.Errorf("undated volume carries created_at: %v", rows[1]) + } + if rows[0]["name"] != "box-a-os" || rows[0]["size"] != float64(50) { + t.Errorf("identity fields lost: %v", rows[0]) + } +} + +// DeletedAt drives the 96-hour recovery countdown, so a fabricated date here +// would misreport how long a volume can still be restored. +func TestTrashTableMarksAbsentDeletedAt(t *testing.T) { + t.Parallel() + + got := runTrashCmd(t, trashBody, "table", true) + + if strings.Contains(got, "0001") { + t.Errorf("table emits a zero timestamp:\n%s", got) + } + if !strings.Contains(got, "2 volume(s) in trash") { + t.Errorf("missing the count line:\n%s", got) + } + if !strings.Contains(got, "11 Aug 2026") { + t.Errorf("real deleted_at not rendered:\n%s", got) + } + if !strings.Contains(got, "Deleted: -\n") { + t.Errorf("absent timestamp not rendered as %q:\n%s", "-", got) + } + // No expiry countdown without a deletion date to count from. + if strings.Count(got, "Expires:") != 1 { + t.Errorf("Expires count = %d, want 1 (only the dated row):\n%s", strings.Count(got, "Expires:"), got) + } +} + +// lipgloss renders escapes into a buffer regardless of where the buffer goes; +// agent mode and a non-terminal destination must both suppress them. +func TestTrashTableHasNoANSIWhenNotATerminal(t *testing.T) { + t.Parallel() + + got := runTrashCmd(t, trashBody, "table", true) + if strings.ContainsRune(got, '\033') { + t.Errorf("table output carries ANSI escapes:\n%q", got) + } +} + +func TestTrashEmpty(t *testing.T) { + t.Parallel() + + if got := runTrashCmd(t, `[]`, "table", true); !strings.Contains(got, "Trash is empty.") { + t.Errorf("got %q", got) + } + if got := runTrashCmd(t, `[]`, "json", true); strings.TrimSpace(got) != "[]" { + t.Errorf("empty json = %q, want []", got) + } +} diff --git a/internal/verda-cli/cmd/volume/view.go b/internal/verda-cli/cmd/volume/view.go index 40ce46c..af54524 100644 --- a/internal/verda-cli/cmd/volume/view.go +++ b/internal/verda-cli/cmd/volume/view.go @@ -19,11 +19,25 @@ import ( "charm.land/lipgloss/v2" "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" ) +// trashStyles returns dim/bold/warning styles, or unstyled ones when the +// destination is not a terminal — lipgloss renders escapes into a buffer with no +// knowledge of where that buffer ends up. +func trashStyles(styled bool) (dim, bold, warn lipgloss.Style) { + if !styled { + plain := lipgloss.NewStyle() + return plain, plain, plain + } + return lipgloss.NewStyle().Foreground(lipgloss.Color("8")), + lipgloss.NewStyle().Bold(true), + lipgloss.NewStyle().Foreground(lipgloss.Color("1")) +} + func renderVolumeSummary(w interface{ Write([]byte) (int, error) }, vol *verda.Volume) { - bold := lipgloss.NewStyle().Bold(true) - dim := lipgloss.NewStyle().Foreground(lipgloss.Color("8")) + dim, bold, _ := trashStyles(cmdutil.IsStdoutTerminal()) status := vol.Status if vol.IsOSVolume { From 5b70d6c9e5730897fad879e8d6aaaf31ef376bbf Mon Sep 17 00:00:00 2001 From: lei Date: Thu, 13 Aug 2026 10:47:41 +0300 Subject: [PATCH 14/18] docs(cost): position CLI money figures as estimates, not billing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard is the billing source of truth. The CLI aligns with catalog prices so planning matches, but it claimed more than that: `verda cost` advertised "billing", `cost running` said it would "calculate the cost" of running instances, and the summary line read "Total Burn" as though it were a charge. That total is a plain sum of catalog price_per_hour plus volume base_hourly_cost — it cannot see credits, discounts, contract terms or partial hours, so any of those makes it disagree with the console, and a CLI that disagrees about money reads as a billing bug. Reword to estimates, point at the dashboard as the authority, and label the summary "Est. Burn" with a one-line footer. No arithmetic changed. Test pins the wording, since positioning text regresses silently. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/cost/cost.go | 9 +++++-- internal/verda-cli/cmd/cost/running.go | 18 ++++++++----- internal/verda-cli/cmd/cost/running_test.go | 29 +++++++++++++++++++++ 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/internal/verda-cli/cmd/cost/cost.go b/internal/verda-cli/cmd/cost/cost.go index 472bbdc..91b5f27 100644 --- a/internal/verda-cli/cmd/cost/cost.go +++ b/internal/verda-cli/cmd/cost/cost.go @@ -24,9 +24,14 @@ import ( func NewCmdCost(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command { cmd := &cobra.Command{ Use: "cost", - Short: "Cost estimation, pricing, and billing", + Short: "Cost estimates, pricing, and account balance", Long: cmdutil.LongDesc(` - Estimate costs, view price history, and check account balance. + Estimate costs, view pricing, and check account balance. + + Figures here are estimates built from catalog prices, for planning. + The Verda dashboard is the authority on what you are charged: it + accounts for credits, discounts and contract terms this CLI cannot + see. Where the two differ, the dashboard is right. `), Run: cmdutil.DefaultSubCommandRun(ioStreams.Out), } diff --git a/internal/verda-cli/cmd/cost/running.go b/internal/verda-cli/cmd/cost/running.go index 0afa403..ddeb09c 100644 --- a/internal/verda-cli/cmd/cost/running.go +++ b/internal/verda-cli/cmd/cost/running.go @@ -29,11 +29,14 @@ import ( func newCmdRunning(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command { return &cobra.Command{ Use: "running", - Short: "Show costs of currently running instances", + Short: "Estimate the ongoing cost of running instances", Long: cmdutil.LongDesc(` - Calculate the cost of all currently running instances, - including their attached volumes. Shows per-instance - breakdown and total burn rate. + Estimate what currently running instances and their attached volumes + cost per hour, day and month, with a per-instance breakdown. + + This is a sum of catalog prices, not an invoice: it excludes credits, + discounts, contract terms and partial-hour handling. Check the Verda + dashboard for actual charges. `), Example: cmdutil.Examples(` verda cost running @@ -211,9 +214,12 @@ func renderRunning(w interface{ Write([]byte) (int, error) }, s *RunningCostSumm _, _ = fmt.Fprintf(w, " %s\n", sep) _, _ = fmt.Fprintf(w, " %s %s/hr %s/day %s/mo\n", - bold.Render("Total Burn"), + bold.Render("Est. Burn "), bold.Render(price.Render(formatPrice(s.Total.Hourly))), bold.Render(price.Render(formatPrice(s.Total.Daily))), bold.Render(price.Render(formatPrice(s.Total.Monthly)))) - _, _ = fmt.Fprintf(w, " %s\n\n", sep) + _, _ = fmt.Fprintf(w, " %s\n", sep) + // The dashboard owns billing; this total is catalog arithmetic. + _, _ = fmt.Fprintf(w, " %s\n\n", + dim.Render("Estimate from catalog prices — see the Verda dashboard for actual charges.")) } diff --git a/internal/verda-cli/cmd/cost/running_test.go b/internal/verda-cli/cmd/cost/running_test.go index 2a0d047..f826df5 100644 --- a/internal/verda-cli/cmd/cost/running_test.go +++ b/internal/verda-cli/cmd/cost/running_test.go @@ -15,6 +15,7 @@ package cost import ( + "strings" "testing" "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" @@ -102,3 +103,31 @@ func TestRunningCostSummaryTotals(t *testing.T) { t.Fatalf("expected total monthly 423.4, got %f", s.Total.Monthly) } } + +// The web console is the billing source of truth, so a CLI total must read as an +// estimate. This pins the wording that says so. +func TestRenderRunningLabelsTheTotalAsAnEstimate(t *testing.T) { + t.Parallel() + + s := &RunningCostSummary{ + Instances: []RunningInstanceCost{{ + Hostname: "box-a", InstanceType: "CPU.4V.16G", + Hourly: 0.0279, Daily: 0.6696, Monthly: 20.367, + }}, + } + s.computeTotals() + + var b strings.Builder + renderRunning(&b, s) + out := b.String() + + if !strings.Contains(out, "Est. Burn") { + t.Errorf("total is not labeled as an estimate:\n%s", out) + } + if strings.Contains(out, "Total Burn") { + t.Errorf("total still reads as an authoritative charge:\n%s", out) + } + if !strings.Contains(out, "Verda dashboard for actual charges") { + t.Errorf("missing the pointer to the billing source of truth:\n%s", out) + } +} From 3ebae44c7d122d5ceddf910245ecb45e6a77c8b7 Mon Sep 17 00:00:00 2001 From: lei Date: Thu, 13 Aug 2026 10:55:14 +0300 Subject: [PATCH 15/18] feat(pricing): one disclaimer on every surface that shows a price MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keeps the prices, including the trashed-volume rate, but never lets them read as the amount charged. cmdutil.PriceDisclaimer is a single constant so the wording cannot drift between surfaces, and callers style it — cmdutil stays presentation-free. Applied where a price appears: cost estimate, cost running, instance-types, volume create, volume trash, and the vm create summary, which is the moment a user commits to spend. Structured output is untouched; the disclaimer is human-facing, and agents get it through the MCP instructions instead. Those instructions gain a PRICING paragraph, because that is where the risk of a wrong number actually lives: a model relaying figures can misread or miscalculate them. It tells the agent never to present a figure as the amount charged, to say so when it sums or converts, and to point at the web console. The CLI's own figures are catalog data plus arithmetic, not model output, so the disclaimer says "may be inaccurate" rather than claiming they are AI-generated — a false statement there would undercut the warning it makes. Terminology follows the web console, matching how billing authority is described. Older strings still say "Verda dashboard" for where to create access keys; unifying those is a separate pass. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/cost/cost.go | 4 ++-- internal/verda-cli/cmd/cost/estimate.go | 3 ++- internal/verda-cli/cmd/cost/running.go | 8 +++----- internal/verda-cli/cmd/cost/running_test.go | 4 ++-- internal/verda-cli/cmd/instancetypes/instancetypes.go | 1 + internal/verda-cli/cmd/mcp/server.go | 8 +++++++- internal/verda-cli/cmd/util/pricing.go | 6 ++++++ internal/verda-cli/cmd/vm/wizard_summary.go | 1 + internal/verda-cli/cmd/volume/create.go | 1 + internal/verda-cli/cmd/volume/trash.go | 1 + internal/verda-cli/cmd/volume/trash_test.go | 11 +++++++++++ 11 files changed, 37 insertions(+), 11 deletions(-) diff --git a/internal/verda-cli/cmd/cost/cost.go b/internal/verda-cli/cmd/cost/cost.go index 91b5f27..db68183 100644 --- a/internal/verda-cli/cmd/cost/cost.go +++ b/internal/verda-cli/cmd/cost/cost.go @@ -29,9 +29,9 @@ func NewCmdCost(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command { Estimate costs, view pricing, and check account balance. Figures here are estimates built from catalog prices, for planning. - The Verda dashboard is the authority on what you are charged: it + The web console is the authority on what you are charged: it accounts for credits, discounts and contract terms this CLI cannot - see. Where the two differ, the dashboard is right. + see. Where the two differ, the web console is right. `), Run: cmdutil.DefaultSubCommandRun(ioStreams.Out), } diff --git a/internal/verda-cli/cmd/cost/estimate.go b/internal/verda-cli/cmd/cost/estimate.go index 841b57f..21f8d4b 100644 --- a/internal/verda-cli/cmd/cost/estimate.go +++ b/internal/verda-cli/cmd/cost/estimate.go @@ -283,7 +283,8 @@ func renderEstimate(w interface{ Write([]byte) (int, error) }, e *Estimate) { bold.Render(price.Render(fmt.Sprintf("%10s", formatPrice(e.Total.Hourly)))), bold.Render(price.Render(fmt.Sprintf("%10s", formatPrice(e.Total.Daily)))), bold.Render(price.Render(fmt.Sprintf("%12s", formatPrice(e.Total.Monthly))))) - _, _ = fmt.Fprintf(w, " %s\n\n", sep) + _, _ = fmt.Fprintf(w, " %s\n", sep) + _, _ = fmt.Fprintf(w, " %s\n\n", dim.Render(cmdutil.PriceDisclaimer)) } func renderLine(w interface{ Write([]byte) (int, error) }, label string, item LineItem, priceStyle *lipgloss.Style) { diff --git a/internal/verda-cli/cmd/cost/running.go b/internal/verda-cli/cmd/cost/running.go index ddeb09c..a70e088 100644 --- a/internal/verda-cli/cmd/cost/running.go +++ b/internal/verda-cli/cmd/cost/running.go @@ -35,8 +35,8 @@ func newCmdRunning(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Comman cost per hour, day and month, with a per-instance breakdown. This is a sum of catalog prices, not an invoice: it excludes credits, - discounts, contract terms and partial-hour handling. Check the Verda - dashboard for actual charges. + discounts, contract terms and partial-hour handling. Check the web console + for actual charges. `), Example: cmdutil.Examples(` verda cost running @@ -219,7 +219,5 @@ func renderRunning(w interface{ Write([]byte) (int, error) }, s *RunningCostSumm bold.Render(price.Render(formatPrice(s.Total.Daily))), bold.Render(price.Render(formatPrice(s.Total.Monthly)))) _, _ = fmt.Fprintf(w, " %s\n", sep) - // The dashboard owns billing; this total is catalog arithmetic. - _, _ = fmt.Fprintf(w, " %s\n\n", - dim.Render("Estimate from catalog prices — see the Verda dashboard for actual charges.")) + _, _ = fmt.Fprintf(w, " %s\n\n", dim.Render(cmdutil.PriceDisclaimer)) } diff --git a/internal/verda-cli/cmd/cost/running_test.go b/internal/verda-cli/cmd/cost/running_test.go index f826df5..d5b375a 100644 --- a/internal/verda-cli/cmd/cost/running_test.go +++ b/internal/verda-cli/cmd/cost/running_test.go @@ -127,7 +127,7 @@ func TestRenderRunningLabelsTheTotalAsAnEstimate(t *testing.T) { if strings.Contains(out, "Total Burn") { t.Errorf("total still reads as an authoritative charge:\n%s", out) } - if !strings.Contains(out, "Verda dashboard for actual charges") { - t.Errorf("missing the pointer to the billing source of truth:\n%s", out) + if !strings.Contains(out, cmdutil.PriceDisclaimer) { + t.Errorf("missing the shared price disclaimer:\n%s", out) } } diff --git a/internal/verda-cli/cmd/instancetypes/instancetypes.go b/internal/verda-cli/cmd/instancetypes/instancetypes.go index 7ca932f..3f16446 100644 --- a/internal/verda-cli/cmd/instancetypes/instancetypes.go +++ b/internal/verda-cli/cmd/instancetypes/instancetypes.go @@ -167,6 +167,7 @@ func renderTypes(w interface{ Write([]byte) (int, error) }, types []verda.Instan } } + _, _ = fmt.Fprintf(w, "\n %s\n", dim.Render(cmdutil.PriceDisclaimer)) _, _ = fmt.Fprintln(w) } diff --git a/internal/verda-cli/cmd/mcp/server.go b/internal/verda-cli/cmd/mcp/server.go index 96b8155..6c54929 100644 --- a/internal/verda-cli/cmd/mcp/server.go +++ b/internal/verda-cli/cmd/mcp/server.go @@ -78,7 +78,13 @@ details.api_message, when present, is the upstream text kept verbatim. STATUS HONESTY — create/action tools return status "accepted" unless you pass wait: true, which polls and returns "completed". Never tell the user a resource -is ready on an "accepted" result.` +is ready on an "accepted" result. + +PRICING — every figure these tools return is an estimate from the catalog. It +cannot see credits, discounts or contract terms, and you may misread or +miscalculate it. Never present a number as the amount the user will be charged, +never sum or convert figures for them without saying you did, and always point +them at the web console, which is authoritative for charges.` func newServer(getClient clientFunc) *Server { s := &Server{getClient: getClient} diff --git a/internal/verda-cli/cmd/util/pricing.go b/internal/verda-cli/cmd/util/pricing.go index 8a0cab7..e3971ff 100644 --- a/internal/verda-cli/cmd/util/pricing.go +++ b/internal/verda-cli/cmd/util/pricing.go @@ -25,6 +25,12 @@ import ( // matching the web frontend's hoursInMonth. const HoursInMonth = 730 +// PriceDisclaimer accompanies every rendered price. The web console owns billing; +// catalog arithmetic here cannot see credits, discounts or contract terms, so it +// must never read as the amount charged. One constant so the wording cannot +// drift between surfaces; callers style it (cmdutil stays presentation-free). +const PriceDisclaimer = "Prices are estimates from the catalog and may be inaccurate — the web console is authoritative for charges." + // VolumeHourlyPrice converts volume pricing (monthlyPerGB per GiB) to the // hourly rate for a sizeGB volume: monthlyPerGB*sizeGB spread over the month, // rounded up to 4 decimals. The ceiling is applied AFTER multiplying by size — diff --git a/internal/verda-cli/cmd/vm/wizard_summary.go b/internal/verda-cli/cmd/vm/wizard_summary.go index 929713b..0658b90 100644 --- a/internal/verda-cli/cmd/vm/wizard_summary.go +++ b/internal/verda-cli/cmd/vm/wizard_summary.go @@ -174,6 +174,7 @@ func renderDeploymentSummary(opts *createOptions, cache *apiCache) string { total := computeHourly + storageHourly fmt.Fprintf(&b, " %s %s\n", bold.Render(fmt.Sprintf("%-40s", "Total")), bold.Render(fmt.Sprintf("$%.4f/hr", total))) fmt.Fprintf(&b, " %s\n", dim.Render(strings.Repeat("─", 50))) + fmt.Fprintf(&b, " %s\n", dim.Render(cmdutil.PriceDisclaimer)) return b.String() } diff --git a/internal/verda-cli/cmd/volume/create.go b/internal/verda-cli/cmd/volume/create.go index c42b98c..0c358fd 100644 --- a/internal/verda-cli/cmd/volume/create.go +++ b/internal/verda-cli/cmd/volume/create.go @@ -176,6 +176,7 @@ func runCreate(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStream _, _ = fmt.Fprintf(ioStreams.ErrOut, " %-30s %s\n", "Unit price", priceStyle.Render(fmt.Sprintf("$%.2f/GB/mo", monthlyPerGB))) _, _ = fmt.Fprintf(ioStreams.ErrOut, " %-30s %s\n", "Monthly", priceStyle.Render(fmt.Sprintf("$%.2f/mo", monthly))) _, _ = fmt.Fprintf(ioStreams.ErrOut, " %s %s\n", bold.Render(fmt.Sprintf("%-30s", "Hourly")), bold.Render(priceStyle.Render(fmt.Sprintf("$%.4f/hr", hourly)))) + _, _ = fmt.Fprintf(ioStreams.ErrOut, " %s\n", dim.Render(cmdutil.PriceDisclaimer)) _, _ = fmt.Fprintf(ioStreams.ErrOut, " %s\n\n", dim.Render(strings.Repeat("─", 45))) if !opts.Yes { diff --git a/internal/verda-cli/cmd/volume/trash.go b/internal/verda-cli/cmd/volume/trash.go index 0cd57f5..c15ea0b 100644 --- a/internal/verda-cli/cmd/volume/trash.go +++ b/internal/verda-cli/cmd/volume/trash.go @@ -115,6 +115,7 @@ func runTrash(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams } _, _ = fmt.Fprintln(&b) } + _, _ = fmt.Fprintf(&b, " %s\n\n", dim.Render(cmdutil.PriceDisclaimer)) // Use pager for scrollable output when list is long. if status := f.Status(); status != nil { diff --git a/internal/verda-cli/cmd/volume/trash_test.go b/internal/verda-cli/cmd/volume/trash_test.go index 069d8da..7d7173d 100644 --- a/internal/verda-cli/cmd/volume/trash_test.go +++ b/internal/verda-cli/cmd/volume/trash_test.go @@ -153,3 +153,14 @@ func TestTrashEmpty(t *testing.T) { t.Errorf("empty json = %q, want []", got) } } + +// Trashed volumes report a monthly_price, so the table must carry the disclaimer +// that the web console — not this output — is authoritative for charges. +func TestTrashTableCarriesPriceDisclaimer(t *testing.T) { + t.Parallel() + + got := runTrashCmd(t, trashBody, "table", true) + if !strings.Contains(got, cmdutil.PriceDisclaimer) { + t.Errorf("missing the price disclaimer:\n%s", got) + } +} From 3391f4084f4a6065e694aae132c7bdf0ebfcd135 Mon Sep 17 00:00:00 2001 From: lei Date: Thu, 13 Aug 2026 11:03:19 +0300 Subject: [PATCH 16/18] refactor(pricing): shorten the disclaimer, pin it out of structured output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two elements are load-bearing — it is an estimate, and the web console decides charges — so the rest went: 107 characters down to 57. It sits under tables and summaries, where a long line competes with the numbers it qualifies. Tests now assert the disclaimer is absent from JSON, in cost estimate and volume trash, so nobody adds it to the machine contract later. Co-Authored-By: Claude Opus 5 (1M context) --- internal/verda-cli/cmd/cost/estimate_test.go | 24 ++++++++++++++++++++ internal/verda-cli/cmd/util/pricing.go | 7 +++--- internal/verda-cli/cmd/volume/trash_test.go | 4 ++++ 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/internal/verda-cli/cmd/cost/estimate_test.go b/internal/verda-cli/cmd/cost/estimate_test.go index 2c672f4..c70607e 100644 --- a/internal/verda-cli/cmd/cost/estimate_test.go +++ b/internal/verda-cli/cmd/cost/estimate_test.go @@ -15,6 +15,7 @@ package cost import ( + "bytes" "math" "strings" "testing" @@ -187,3 +188,26 @@ func TestEstimateTotals(t *testing.T) { t.Fatalf("expected total monthly $381.20, got $%.2f", e.Total.Monthly) } } + +// The disclaimer belongs in human output only: adding it to JSON/YAML would +// change the contract agents parse. +func TestEstimateStructuredOutputHasNoDisclaimer(t *testing.T) { + t.Parallel() + + e := Estimate{ + InstanceType: "CPU.4V.16G", + Instance: LineItem{Hourly: 0.0279, Daily: 0.6696, Monthly: 20.367}, + } + e.computeTotals() + + var buf bytes.Buffer + if _, err := cmdutil.WriteStructured(&buf, "json", e); err != nil { + t.Fatalf("WriteStructured: %v", err) + } + if strings.Contains(buf.String(), cmdutil.PriceDisclaimer) { + t.Errorf("disclaimer leaked into JSON:\n%s", buf.String()) + } + if strings.Contains(buf.String(), "disclaimer") { + t.Errorf("JSON gained a disclaimer field:\n%s", buf.String()) + } +} diff --git a/internal/verda-cli/cmd/util/pricing.go b/internal/verda-cli/cmd/util/pricing.go index e3971ff..626fb94 100644 --- a/internal/verda-cli/cmd/util/pricing.go +++ b/internal/verda-cli/cmd/util/pricing.go @@ -27,9 +27,10 @@ const HoursInMonth = 730 // PriceDisclaimer accompanies every rendered price. The web console owns billing; // catalog arithmetic here cannot see credits, discounts or contract terms, so it -// must never read as the amount charged. One constant so the wording cannot -// drift between surfaces; callers style it (cmdutil stays presentation-free). -const PriceDisclaimer = "Prices are estimates from the catalog and may be inaccurate — the web console is authoritative for charges." +// must never read as the amount charged. One constant so the wording cannot drift +// between surfaces; callers style it (cmdutil stays presentation-free). +// Human-facing only — structured output carries no disclaimer field. +const PriceDisclaimer = "Estimates — the web console is authoritative for charges." // VolumeHourlyPrice converts volume pricing (monthlyPerGB per GiB) to the // hourly rate for a sizeGB volume: monthlyPerGB*sizeGB spread over the month, diff --git a/internal/verda-cli/cmd/volume/trash_test.go b/internal/verda-cli/cmd/volume/trash_test.go index 7d7173d..5c2775e 100644 --- a/internal/verda-cli/cmd/volume/trash_test.go +++ b/internal/verda-cli/cmd/volume/trash_test.go @@ -105,6 +105,10 @@ func TestTrashHonorsJSONOutput(t *testing.T) { if rows[0]["name"] != "box-a-os" || rows[0]["size"] != float64(50) { t.Errorf("identity fields lost: %v", rows[0]) } + // The disclaimer is human-facing; it must never enter the machine contract. + if strings.Contains(got, cmdutil.PriceDisclaimer) { + t.Errorf("disclaimer leaked into structured output:\n%s", got) + } } // DeletedAt drives the 96-hour recovery countdown, so a fabricated date here From f7d6920fa77e358106960f972ce17d0d04bce80d Mon Sep 17 00:00:00 2001 From: lei Date: Thu, 13 Aug 2026 11:30:04 +0300 Subject: [PATCH 17/18] fix(output): strip ANSI at the stream boundary, not per command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #4 was closed against `verda locations`, which builds its table with plain Fprintf and could never have shown the defect. Measured on the built binary redirected to a file: instance-types 68 escape-carrying lines, status 12, cost estimate 11, availability 7, cost balance 1. Structurally, 21 files style output on the data stream with no terminal check, so `verda instance-types | jq` and `> file` both received escapes. The per-command gate would have been 21 edits against a defect that is not per command. lipgloss v2 emits escapes from Style.Render unconditionally — a Style cannot know its destination — and delegates stripping to a writer. So wrap Out and ErrOut once in NewStdIOStreams with colorprofile.NewWriter, which detects what the destination can display and downsamples or strips to match. Existing IsStdoutTerminal checks still choose layout; this decides color. Tests build IOStreams directly and keep raw buffers, so none of them change meaning. Verified live: those five commands now write 0 escapes with content intact. The color-preserved direction has no PTY in this sandbox, so it is covered by a test that forces a color-capable profile instead. Hardening, since one wrap is easy to undo by accident: tests/contract/ansi_purity_test.go runs the real binary with stdout piped across 11 table-rendering commands plus 4 -o json paths, asserting no escapes, a zero exit and non-empty output so a failed run cannot pass vacuously. Forcing TrueColor onto the pipe fails 7 of them — the exact commands that were leaking. cmd/util/iostreams_test.go pins the wiring itself, both profile directions, and the premise that Render always styles. colorprofile moves from indirect to direct in go.mod: same v0.4.2 already in the tree via lipgloss, no new module. Co-Authored-By: Claude Opus 5 (1M context) --- go.mod | 2 +- internal/verda-cli/cmd/util/iostreams.go | 14 +- internal/verda-cli/cmd/util/iostreams_test.go | 127 ++++++++++++++++++ tests/contract/ansi_purity_test.go | 124 +++++++++++++++++ 4 files changed, 264 insertions(+), 3 deletions(-) create mode 100644 internal/verda-cli/cmd/util/iostreams_test.go create mode 100644 tests/contract/ansi_purity_test.go diff --git a/go.mod b/go.mod index 647e12f..6c4caa6 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.22.15 github.com/aws/aws-sdk-go-v2/service/s3 v1.99.1 github.com/aws/smithy-go v1.25.0 + github.com/charmbracelet/colorprofile v0.4.2 github.com/charmbracelet/x/term v0.2.2 github.com/google/go-containerregistry v0.21.5 github.com/mark3labs/mcp-go v0.47.0 @@ -45,7 +46,6 @@ require ( github.com/aws/aws-sdk-go-v2/service/ssooidc v1.35.20 // indirect github.com/aws/aws-sdk-go-v2/service/sts v1.42.0 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect - github.com/charmbracelet/colorprofile v0.4.2 // indirect github.com/charmbracelet/harmonica v0.2.0 // indirect github.com/charmbracelet/ultraviolet v0.0.0-20260205113103-524a6607adb8 // indirect github.com/charmbracelet/x/ansi v0.11.6 // indirect diff --git a/internal/verda-cli/cmd/util/iostreams.go b/internal/verda-cli/cmd/util/iostreams.go index cd6c1f6..f84ff1a 100644 --- a/internal/verda-cli/cmd/util/iostreams.go +++ b/internal/verda-cli/cmd/util/iostreams.go @@ -18,6 +18,7 @@ import ( "io" "os" + "github.com/charmbracelet/colorprofile" "github.com/charmbracelet/x/term" ) @@ -30,11 +31,20 @@ type IOStreams struct { } // NewStdIOStreams returns an IOStreams wired to os.Stdin, os.Stdout, and os.Stderr. +// +// Both writers are wrapped in a colorprofile writer, which detects what the +// destination can display and downsamples or strips ANSI accordingly. lipgloss +// v2 always emits escapes from Style.Render — stripping is the writer's job, not +// the style's — so this is the single point where a piped or redirected stream +// stops receiving color. Per-command IsStdoutTerminal checks remain useful for +// choosing a *layout* (interactive picker vs plain list); this handles color. +// +// Tests build IOStreams directly, so they keep raw buffers and are unaffected. func NewStdIOStreams() IOStreams { return IOStreams{ In: os.Stdin, - Out: os.Stdout, - ErrOut: os.Stderr, + Out: colorprofile.NewWriter(os.Stdout, os.Environ()), + ErrOut: colorprofile.NewWriter(os.Stderr, os.Environ()), } } diff --git a/internal/verda-cli/cmd/util/iostreams_test.go b/internal/verda-cli/cmd/util/iostreams_test.go new file mode 100644 index 0000000..e26a90d --- /dev/null +++ b/internal/verda-cli/cmd/util/iostreams_test.go @@ -0,0 +1,127 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package util + +import ( + "bytes" + "fmt" + "os" + "strings" + "testing" + + "charm.land/lipgloss/v2" + "github.com/charmbracelet/colorprofile" +) + +// styled is what every table and card in this CLI produces: lipgloss always +// emits escapes, whatever the destination turns out to be. +func styled() string { + return lipgloss.NewStyle().Bold(true).Foreground(lipgloss.Color("2")).Render("GPU Instances") +} + +func TestStyleRenderAlwaysEmitsEscapes(t *testing.T) { + t.Parallel() + + // The premise of the wiring: stripping cannot be the style's job, because a + // Style has no idea where its output goes. + if !strings.ContainsRune(styled(), '\033') { + t.Fatal("lipgloss no longer emits escapes; the colorprofile wrapper may be redundant") + } +} + +// A piped or redirected stream must receive plain text: this is the defect that +// reached users as ANSI in `verda instance-types > file`. +func TestColorProfileWriterStripsForNonTerminal(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + w := colorprofile.NewWriter(&buf, os.Environ()) + + if _, err := fmt.Fprint(w, styled()); err != nil { + t.Fatalf("write: %v", err) + } + + got := buf.String() + if strings.ContainsRune(got, '\033') { + t.Errorf("escapes survived to a non-terminal writer: %q", got) + } + if !strings.Contains(got, "GPU Instances") { + t.Errorf("text lost along with the color: %q", got) + } +} + +// The other direction, which a sandbox cannot prove with a real PTY: when the +// destination can display color, the wrapper must not strip it. Forcing the +// profile isolates the writer's behavior from terminal detection. +func TestColorProfileWriterKeepsColorWhenSupported(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + w := &colorprofile.Writer{Forward: &buf, Profile: colorprofile.TrueColor} + + if _, err := fmt.Fprint(w, styled()); err != nil { + t.Fatalf("write: %v", err) + } + + got := buf.String() + if !strings.ContainsRune(got, '\033') { + t.Errorf("color was stripped for a color-capable destination: %q", got) + } + if !strings.Contains(got, "GPU Instances") { + t.Errorf("text lost: %q", got) + } +} + +// Pins the wiring itself: unwrapping either stream silently reintroduces ANSI on +// every piped command, which no per-command test would notice. +func TestNewStdIOStreamsWrapsBothWriters(t *testing.T) { + t.Parallel() + + s := NewStdIOStreams() + + out, ok := s.Out.(*colorprofile.Writer) + if !ok { + t.Fatalf("Out is %T, want *colorprofile.Writer", s.Out) + } + if out.Forward != os.Stdout { + t.Errorf("Out forwards to %v, want os.Stdout", out.Forward) + } + + errOut, ok := s.ErrOut.(*colorprofile.Writer) + if !ok { + t.Fatalf("ErrOut is %T, want *colorprofile.Writer", s.ErrOut) + } + if errOut.Forward != os.Stderr { + t.Errorf("ErrOut forwards to %v, want os.Stderr", errOut.Forward) + } + + if s.In != os.Stdin { + t.Errorf("In = %v, want os.Stdin", s.In) + } +} + +// Under test the profile is derived from the destination, so a buffer resolves to +// something that cannot show color. If this ever flips, the strip test above +// would pass for the wrong reason. +func TestNonTerminalProfileIsDetectedNotAssumed(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + w := colorprofile.NewWriter(&buf, os.Environ()) + + if w.Profile == colorprofile.TrueColor || w.Profile == colorprofile.ANSI256 { + t.Errorf("a bytes.Buffer resolved to %v; detection is not seeing the destination", w.Profile) + } +} diff --git a/tests/contract/ansi_purity_test.go b/tests/contract/ansi_purity_test.go new file mode 100644 index 0000000..1b9a6da --- /dev/null +++ b/tests/contract/ansi_purity_test.go @@ -0,0 +1,124 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package contract + +import ( + "strings" + "testing" + + "github.com/verda-cloud/verda-cli/tests/contract/mockapi" +) + +// Escapes reach a pipe whenever a command styles its own output: lipgloss +// renders them unconditionally, and only the writer knows the destination. This +// suite runs the real binary with stdout piped — the shape every `verda … | jq` +// and `> file` takes — across the commands that render tables and cards. +// +// Per-command tests cannot catch this class: they hand commands a raw +// bytes.Buffer, so they never exercise the wiring that decides on color. +func TestPipedOutputCarriesNoANSI(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + args []string + seed func(*mockapi.Server) + }{ + {name: "instance-types", args: []string{"instance-types"}}, + {name: "instance-types --cpu", args: []string{"instance-types", "--cpu"}}, + {name: "availability", args: []string{"availability"}}, + {name: "locations", args: []string{"locations"}}, + {name: "cost balance", args: []string{"cost", "balance"}}, + {name: "cost estimate", args: []string{"cost", "estimate", "--type", mockapi.TypeCPU}}, + { + name: "cost running", + args: []string{"cost", "running"}, + seed: func(s *mockapi.Server) { + s.SeedInstance("ansi-burn", mockapi.TypeCPU, mockapi.CPUOnDemandTotal) + }, + }, + { + name: "vm list", + args: []string{"vm", "list"}, + seed: func(s *mockapi.Server) { + s.SeedInstance("ansi-alpha", mockapi.TypeCPU, mockapi.CPUOnDemandTotal) + }, + }, + { + name: "volume list", + args: []string{"volume", "list"}, + seed: func(s *mockapi.Server) { s.SeedVolume("ansi-vol", 100) }, + }, + {name: "ssh-key list", args: []string{"ssh-key", "list"}}, + {name: "status", args: []string{"status"}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + srv := newServer(t) + if tc.seed != nil { + tc.seed(srv) + } + + r := runCLI(t, srv, tc.args...) + + if strings.ContainsRune(r.Stdout, '\033') { + t.Errorf("stdout carries ANSI escapes when piped:\n%q", r.Stdout) + } + // Guard against passing for the wrong reason: an empty or failed run + // has no escapes either. + if r.ExitCode != 0 { + t.Errorf("exit = %d, want 0\nstderr: %s", r.ExitCode, r.Stderr) + } + if strings.TrimSpace(r.Stdout) == "" { + t.Errorf("no stdout to inspect; the assertion would be vacuous\nstderr: %s", r.Stderr) + } + }) + } +} + +// Structured output must be parseable byte-for-byte, so the same rule applies +// with -o json — and here an escape would break json.Unmarshal outright. +func TestPipedJSONCarriesNoANSI(t *testing.T) { + t.Parallel() + + srv := newServer(t) + srv.SeedInstance("ansi-json", mockapi.TypeCPU, mockapi.CPUOnDemandTotal) + srv.SeedVolume("ansi-json-vol", 50) + + for _, args := range [][]string{ + {"vm", "list", "-o", "json"}, + {"volume", "list", "-o", "json"}, + {"instance-types", "-o", "json"}, + {"cost", "running", "-o", "json"}, + } { + name := strings.Join(args, " ") + t.Run(name, func(t *testing.T) { + t.Parallel() + r := runCLI(t, srv, args...) + if r.ExitCode != 0 { + t.Fatalf("exit = %d\nstderr: %s", r.ExitCode, r.Stderr) + } + if strings.ContainsRune(r.Stdout, '\033') { + t.Errorf("JSON stdout carries ANSI escapes:\n%q", r.Stdout) + } + if !strings.HasPrefix(strings.TrimSpace(r.Stdout), "[") && + !strings.HasPrefix(strings.TrimSpace(r.Stdout), "{") { + t.Errorf("stdout is not JSON: %q", r.Stdout) + } + }) + } +} From 999e8a3e0e12829468b5ffd001519c6e167cfd7a Mon Sep 17 00:00:00 2001 From: lei Date: Thu, 13 Aug 2026 19:29:35 +0300 Subject: [PATCH 18/18] test(util): annotate the provably-safe uint conversion for gosec fillNonZero increments the counter on entry, so *n >= 1 at the uint branch and the int -> uint64 conversion cannot wrap. gosec (G115) cannot see that, and `make security` mirrors the CI gate, so the finding would fail the release PR. Co-Authored-By: Claude Opus 5 --- internal/verda-cli/cmd/util/views_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/verda-cli/cmd/util/views_test.go b/internal/verda-cli/cmd/util/views_test.go index cee6331..8af383b 100644 --- a/internal/verda-cli/cmd/util/views_test.go +++ b/internal/verda-cli/cmd/util/views_test.go @@ -320,7 +320,8 @@ func fillNonZero(t *testing.T, v reflect.Value, n *int) { case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: v.SetInt(int64(*n)) case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - v.SetUint(uint64(*n)) + // n is incremented from 0 on entry (line above), so it is never negative here. + v.SetUint(uint64(*n)) //nolint:gosec // G115: *n >= 1 by construction case reflect.Float32, reflect.Float64: v.SetFloat(float64(*n) + 0.25) case reflect.Pointer: