Hotfix/v1.8.0 - #58
Merged
Merged
Conversation
The API omits created_at on /v1/ssh-keys and sends fingerprint: null.
Marshaling the SDK struct turned those into 0001-01-01T00:00:00Z and "",
so an age-based reaper ("older than 2h => delete") read every key as
ancient and deleted keys belonging to running jobs.
Add CLI-owned view types that map a zero time.Time to nil so omitempty
drops the field, and render absent values as "-" in table output. One
shared type backs both the CLI and the MCP tools, so the two JSON
contracts cannot drift.
Absent is safe; wrong is dangerous. This hides the damage — the API gap
itself still needs a backend ticket.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing in the tree ever read VERDA_S3_*: the only credential source was the INI file, so CI could not use object storage without writing secrets to disk. API auth already honored VERDA_CLIENT_ID/_SECRET, which made the documented precedence true for API auth and false for S3. Add options.ResolveS3Credentials: load the profile, then overlay VERDA_S3_ACCESS_KEY/_SECRET_KEY/_ENDPOINT/_REGION/_AUTH_MODE per field. Flags still win (NewClient's firstNonEmpty), so the order is flags > env > file. The merge is per field rather than wholesale, so one variable cannot discard a working profile, and an empty variable counts as unset. A missing file or unresolvable path (no HOME in a container) stops being fatal once env alone is complete; HasCredentials() still gates, so a partial set fails loudly. show uses the same resolver -- otherwise it would report "not configured" for an env-only setup that ls handles fine -- and names the applied variables under env_overrides:, never their values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vm delete and volume delete both take an optional positional id plus --id, but ssh-key delete and startup-script delete were cobra.NoArgs, so `verda ssh-key delete <uuid>` failed with `unknown command`. Scripts had to special-case these two commands. Switch both to MaximumNArgs(1) and copy the established shape. --id keeps working unchanged. Passing both an argument and --id is a usage error rather than a silent pick — vm's shortcut lets the positional overwrite --id, which hides a typo that targets the wrong resource. The agent-mode --yes guard still fires before any API call, whichever way the id arrived. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pre-existing violation, not introduced here: "delete" appears 12 times in cmd/vm (1 in batch.go, 11 in tests). goconst counts across the package but only reports on non-test files, so it lands on batch.go:278. golangci-lint's cache had been hiding it; any edit to the package surfaces it and fails make lint. Reported separately from the help-text change it blocked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Text only, no behavior change. instance-types said "List all available instance types", which reads as live stock and is the reading users act on. It is a catalog; point at verda availability for what can actually be deployed. --os-volume-name and --os-volume-on-spot-discontinue now state that they require --os-volume-size (enforced at create.go:351, previously only discoverable by hitting the error). --description states the 100-character cap, which until now surfaced only as an API 400. No client-side validation added on purpose: a client-side cap that drifts from the API is worse than the 400. --with-volumes states that without it the OS volume survives detached and keeps billing. Updated in both places it is defined (shortcuts.go and action.go) so the two surfaces cannot disagree. Also documents the new positional delete id in the ssh-key and startup-script READMEs, completing aa68b72. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage audit of the four fixes found three gaps, all closed here: object-storage show read credentials through its own call site, and no test exercised the env path there — an env-only setup reporting "not configured" would have gone unnoticed. Three tests: env-only is reported configured, env_overrides names variables and never values (with per-field merge asserted), and nothing-configured still says so. Every S3 verb (ls/cp/mv/sync/rm/mb/rb/presign) shares buildClientDefault, but every existing test swaps in a fake client, so the real builder's env path was never executed. Two tests call it directly: env-only credentials build a client, and no credentials keep the configure hint. MCP add_ssh_key returns the created key to the agent — the second place a zero timestamp reaches a reaper — and only list_ssh_keys was covered. Verified non-vacuous by mutation: disabling the env overlay fails 10 tests (4 new), and making the view emit a zero time fails 7 across cmd/util, cmd/mcp and cmd/sshkey, including the new add test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same defect as be842d4, found by auditing the surfaces rather than the diff, and worse: /v1/instances omits created_at too (the captured staging payload temp/docs/c1-ondemand-instance.json has no time-valued key at all), and verda.Instance.CreatedAt has no omitempty. So `vm list -o json` dated every instance to 0001-01-01, and an age-based reaper reading that deletes running instances, not SSH keys. Six surfaces, all agent-facing: vm list, vm describe, vm create --agent, MCP list_vms, MCP describe_vm, and serverless batchjob list (hidden feature; fixed so it does not ship with the defect). InstanceView mirrors all 24 verda.Instance fields explicitly rather than embedding the SDK struct: embedding plus a shadowed CreatedAt is correct in JSON but leaks `createdat: 0001-01-01T00:00:00Z` in YAML, because yaml.v3 inlines the embedded struct and ignores json tags. Verified both ways before choosing. TestInstanceViewCoversSDKFields compares json tag sets by reflection so the SDK cannot grow a field the view silently drops. Tests fail before and pass after: 8 tests across cmd/util, cmd/vm, cmd/mcp and cmd/serverless catch a mutation that makes the view emit the zero time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestInstanceViewCoversSDKFields compares json tag names, which cannot see a field InstanceView declares but NewInstanceView never assigns: it marshals as a zero value and the guard passes — the very defect class these views exist to prevent, and exactly where a hand-mirrored 24-field constructor goes wrong. Fill every verda.Instance field with a distinctive non-zero value by reflection, marshal SDK struct and view, compare the decoded maps key-by-key. Plus a zero-instance check that created_at is the only key dropped, so no other field can silently leave the agent contract. Verified necessary, not redundant: deleting the Hostname assignment from NewInstanceView fails TestInstanceViewCopiesEveryValue with `key "hostname": view has "", SDK has "s21"` while the tag-name guard still passes. Hole identified by session 862d36ab's cross-check of c27e9df; probes adopted as permanent tests and credited in the source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reported 100-character cap is not real. Verified live against staging: `vm create --description <101 chars>` was accepted and the full 101-character value came back in the instance payload. My earlier help text stated a limit the API does not enforce, which is worse than saying nothing — it would push users to truncate valid input. Reverts that clause from 03d45e5 and records the measurement inline so nobody re-adds it from the original report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… 400 F3 exposed a structural problem, not a missing special case. The agent error contract existed twice: cmdutil.AgentError + ClassifyError on the CLI side, and a private argError twin in the MCP server whose renderer only understood its own type. Every API failure therefore reached agents over MCP as a bare string with no code, while docs/agent-errors.md claimed MCP "reuses this contract". Collapse it onto one type. MCP's argument-error constructors now return *cmdutil.AgentError (keeping MCP's "argument" wording), and toolErrorResult renders every failure through ClassifyError. An API 404 now reaches an agent as NOT_FOUND over MCP exactly as on the CLI, and a code added to the classifier appears on both surfaces without touching either renderer. With one funnel, the ssh-key case is a mapping rather than a hook: classifyAPIError recognizes the 400 whose text cannot be shown to a user. POST /instances rejects a request that omits ssh_key_ids with "SSH keys can be an array of UUID's, a single UUID string, null value or not defined" -- while the field was not defined. Verified live on staging with the request body captured via --debug: no ssh_key_ids was sent, so the CLI was right and the server contradicts itself. Users now get SSH_KEY_REQUIRED, exit 2, naming the flag and how to list ids, with the server's wording preserved verbatim in details.api_message. Live: `vm create` without --ssh-key exits 2 with the new envelope; no resource created. Also fixes two tests that passed silently when their errors.As assertion failed. Contract change: MCP tool errors that used to be plain text now carry the envelope. Documented, with the remaining 45 handler call sites that bypass toolErrorResult tracked as follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Non-agent mode read err.Error() directly, so the classifier's work only reached --agent callers: a human running `vm create` without --ssh-key still saw the API's self-contradictory "SSH keys can be ... null value or not defined", while an agent got SSH_KEY_REQUIRED plus remediation. Now main classifies once and renders twice — envelope for agents, the same classified message as text for humans. Verified live: the human path prints the actionable message. Human failures stay exit 1. Routing ae.ExitCode there would give scripts 2/3/4/5/6 without --agent, which is the better interface but changes what `$?` means for existing callers; that is a separate decision. MCP now ships server instructions in the initialize response, so every client learns the confirm gate, the error envelope and the accepted vs completed rule before its first tool call, with no client-side change. This is the answer to "how do callers learn the new error shape": they are told, rather than asked to upgrade. Old consumers keep working — the envelope contains the same human message and isError is unchanged. Also tightens the comments added across this branch to the repo's style (design and invariants, one line, no change history — that belongs here), and drops a test comment repeating the claim that /v1/instances omits created_at, which live verification disproved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pager launched an alt-screen Bubble Tea program regardless of destination, so `verda volume trash | head` blocked forever waiting for keys nobody could send. terminalHeight falls back to 24 for a pipe, which makes the trap data-dependent: a short trash list took the print-through path and looked fine, while a full one hung. Automation without --agent was the only victim, since agent mode leaves Status nil and never reaches the pager. Guard on rendersToTerminal and print through, matching Spinner and progress, which already do exactly this. volume trash is the only caller today, and the wizard docs advertise Pager to future ones. Test fails before by timing out after 5s and passes instantly after; live, the piped repro now returns the listing immediately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
volume trash never consulted f.OutputFormat() or f.AgentMode(), so `--agent -o json` returned an ANSI-styled table: 40 escape-carrying lines into a redirected stdout. Checking the package turned up the same two defects spread wider than the reported finding. Zero-timestamp class, as in be842d4 and c27e9df: Volume.CreatedAt, VolumeInTrash.CreatedAt and DeletedAt carry no omitempty, and four sites marshal those types raw (list, describe, create, and trash which had no structured branch at all). DeletedAt is the one that matters most — it drives the 96-hour recovery countdown, so a fabricated date misreports how long a volume can still be restored. trash.go already guarded the expiry calculation with !DeletedAt.IsZero() while formatting the date unconditionally two lines above. Add VolumeView and VolumeInTrashView with both guards that earned their keep on InstanceView: json-tag drift and value-copy by reflection. Wire all four sites, render an absent timestamp as "-" via TimeColumn, and gate styling on the expression already used in cp.go, container_list.go and vm/list.go rather than inventing a second one. renderVolumeSummary gets the same gate, since describe and create share it. First trash_test.go in the package. All four tests fail under mutation (structured branch removed, styling forced on). Live: `volume trash --agent -o json` is valid JSON with 0 ANSI lines, table mode redirected is also 0, both exit 0. Closes F2 (order 007). volume trash has no restore path in the CLI or the SDK, so being read by a human or a script is its only purpose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dashboard is the billing source of truth. The CLI aligns with catalog prices so planning matches, but it claimed more than that: `verda cost` advertised "billing", `cost running` said it would "calculate the cost" of running instances, and the summary line read "Total Burn" as though it were a charge. That total is a plain sum of catalog price_per_hour plus volume base_hourly_cost — it cannot see credits, discounts, contract terms or partial hours, so any of those makes it disagree with the console, and a CLI that disagrees about money reads as a billing bug. Reword to estimates, point at the dashboard as the authority, and label the summary "Est. Burn" with a one-line footer. No arithmetic changed. Test pins the wording, since positioning text regresses silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keeps the prices, including the trashed-volume rate, but never lets them read as the amount charged. cmdutil.PriceDisclaimer is a single constant so the wording cannot drift between surfaces, and callers style it — cmdutil stays presentation-free. Applied where a price appears: cost estimate, cost running, instance-types, volume create, volume trash, and the vm create summary, which is the moment a user commits to spend. Structured output is untouched; the disclaimer is human-facing, and agents get it through the MCP instructions instead. Those instructions gain a PRICING paragraph, because that is where the risk of a wrong number actually lives: a model relaying figures can misread or miscalculate them. It tells the agent never to present a figure as the amount charged, to say so when it sums or converts, and to point at the web console. The CLI's own figures are catalog data plus arithmetic, not model output, so the disclaimer says "may be inaccurate" rather than claiming they are AI-generated — a false statement there would undercut the warning it makes. Terminology follows the web console, matching how billing authority is described. Older strings still say "Verda dashboard" for where to create access keys; unifying those is a separate pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…utput Two elements are load-bearing — it is an estimate, and the web console decides charges — so the rest went: 107 characters down to 57. It sits under tables and summaries, where a long line competes with the numbers it qualifies. Tests now assert the disclaimer is absent from JSON, in cost estimate and volume trash, so nobody adds it to the machine contract later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Issue #4 was closed against `verda locations`, which builds its table with plain Fprintf and could never have shown the defect. Measured on the built binary redirected to a file: instance-types 68 escape-carrying lines, status 12, cost estimate 11, availability 7, cost balance 1. Structurally, 21 files style output on the data stream with no terminal check, so `verda instance-types | jq` and `> file` both received escapes. The per-command gate would have been 21 edits against a defect that is not per command. lipgloss v2 emits escapes from Style.Render unconditionally — a Style cannot know its destination — and delegates stripping to a writer. So wrap Out and ErrOut once in NewStdIOStreams with colorprofile.NewWriter, which detects what the destination can display and downsamples or strips to match. Existing IsStdoutTerminal checks still choose layout; this decides color. Tests build IOStreams directly and keep raw buffers, so none of them change meaning. Verified live: those five commands now write 0 escapes with content intact. The color-preserved direction has no PTY in this sandbox, so it is covered by a test that forces a color-capable profile instead. Hardening, since one wrap is easy to undo by accident: tests/contract/ansi_purity_test.go runs the real binary with stdout piped across 11 table-rendering commands plus 4 -o json paths, asserting no escapes, a zero exit and non-empty output so a failed run cannot pass vacuously. Forcing TrueColor onto the pipe fails 7 of them — the exact commands that were leaking. cmd/util/iostreams_test.go pins the wiring itself, both profile directions, and the premise that Render always styles. colorprofile moves from indirect to direct in go.mod: same v0.4.2 already in the tree via lipgloss, no new module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fillNonZero increments the counter on entry, so *n >= 1 at the uint branch and the int -> uint64 conversion cannot wrap. gosec (G115) cannot see that, and `make security` mirrors the CI gate, so the finding would fail the release PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Checklist
make test)make pre-commit)