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/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/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/cost/cost.go b/internal/verda-cli/cmd/cost/cost.go index 472bbdc..db68183 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 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 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/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/cost/running.go b/internal/verda-cli/cmd/cost/running.go index 0afa403..a70e088 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 web console + for actual charges. `), Example: cmdutil.Examples(` verda cost running @@ -211,9 +214,10 @@ 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) + _, _ = 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 2a0d047..d5b375a 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, 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 85bb939..3f16446 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 @@ -163,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 3cc3f91..6c54929 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. @@ -52,6 +53,39 @@ 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. + +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} @@ -59,6 +93,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() @@ -99,59 +136,56 @@ 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 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) *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 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 { - 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..0fb4652 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,14 @@ 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: `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) + } + if ae.Code != "MISSING_REQUIRED_FLAGS" { + t.Errorf("code = %q, want MISSING_REQUIRED_FLAGS", ae.Code) } a["num"] = float64(7) @@ -86,8 +95,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 +232,46 @@ func TestToolErrorResultEnvelope(t *testing.T) { t.Errorf("details.action = %v, want create_vm", env.Error.Details["action"]) } - // Non-argError falls back to plain text. + // Every error carries the envelope, not only MCP's own argument errors. 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) + } + + // 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", + }) + 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_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..af20a01 --- /dev/null +++ b/internal/verda-cli/cmd/mcp/tools_ssh_test.go @@ -0,0 +1,189 @@ +// 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) + } +} + +// 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/mcp/tools_vm.go b/internal/verda-cli/cmd/mcp/tools_vm.go index 304bfa5..40ca2f4 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. @@ -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 + // Envelope, not a bare string: agents branch on create failures + // (SSH_KEY_REQUIRED, INSUFFICIENT_BALANCE). + return toolErrorResult(err), nil } if wait { 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/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..1ecba38 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,196 @@ 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, + } +} + +// 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.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/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) + } +} 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/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/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/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/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/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/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") + } +} 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/agent_error.go b/internal/verda-cli/cmd/util/agent_error.go index 39e392d..b92fe13 100644 --- a/internal/verda-cli/cmd/util/agent_error.go +++ b/internal/verda-cli/cmd/util/agent_error.go @@ -237,11 +237,37 @@ 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 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 + } + 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, + // Upstream text, verbatim, for debugging. + "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..291ee75 100644 --- a/internal/verda-cli/cmd/util/agent_error_test.go +++ b/internal/verda-cli/cmd/util/agent_error_test.go @@ -195,3 +195,41 @@ func TestClassifyError_UsageError(t *testing.T) { t.Error("wrapped UsageError lost the VALIDATION_ERROR classification") } } + +// Unactionable upstream wording becomes a code both surfaces can act on, with +// the original preserved 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/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/internal/verda-cli/cmd/util/pricing.go b/internal/verda-cli/cmd/util/pricing.go index 8a0cab7..626fb94 100644 --- a/internal/verda-cli/cmd/util/pricing.go +++ b/internal/verda-cli/cmd/util/pricing.go @@ -25,6 +25,13 @@ 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). +// 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, // rounded up to 4 decimals. The ceiling is applied AFTER multiplying by size — diff --git a/internal/verda-cli/cmd/util/views.go b/internal/verda-cli/cmd/util/views.go new file mode 100644 index 0000000..41c5899 --- /dev/null +++ b/internal/verda-cli/cmd/util/views.go @@ -0,0 +1,331 @@ +// 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 +} + +// 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"` + 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 +} + +// 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. +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. +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..8af383b --- /dev/null +++ b/internal/verda-cli/cmd/util/views_test.go @@ -0,0 +1,512 @@ +// 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" + "encoding/json" + "reflect" + "strconv" + "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) + } +} + +// 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) + } +} + +// 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: + // 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: + 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 +} + +// 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() + + 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)) + } +} + +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/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/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 diff --git a/internal/verda-cli/cmd/vm/create.go b/internal/verda-cli/cmd/vm/create.go index 7f10134..6345ff3 100644 --- a/internal/verda-cli/cmd/vm/create.go +++ b/internal/verda-cli/cmd/vm/create.go @@ -172,6 +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 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") @@ -185,9 +186,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") @@ -246,7 +247,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..0d3b72c --- /dev/null +++ b/internal/verda-cli/cmd/vm/list_created_at_test.go @@ -0,0 +1,166 @@ +// 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" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" +) + +// 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",` + + `"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) + } +} + +// The upstream 400 contradicts itself; 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") + } +} 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 { 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 b77f94a..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 { @@ -242,7 +243,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..c15ea0b 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) @@ -112,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 { @@ -121,6 +125,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..5c2775e --- /dev/null +++ b/internal/verda-cli/cmd/volume/trash_test.go @@ -0,0 +1,170 @@ +// 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]) + } + // 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 +// 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) + } +} + +// 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) + } +} 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 { 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") + } +} 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()) + } +} 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) + } + }) + } +}