From 9939bec27d03c68e90213c7daff8d7499f7ab2a3 Mon Sep 17 00:00:00 2001 From: Ted Kim Date: Wed, 8 Jul 2026 12:18:29 -0400 Subject: [PATCH 1/6] feat(api): speak the API-controlled CLI version protocol (VOL-180) Replaces the GitHub-release-based update-notice check (VOL-168) with the Volcano Hosting API as the source of truth: the CLI reports its version on every API request, and acts on whatever instruction the API returns, instead of independently polling GitHub. - internal/api: versionProtocolDoer wraps every HTTP call (including unauthenticated ones like device/token exchange) to set X-Volcano-CLI-Version and a real User-Agent (volcano-cli/ (/), replacing Go's default), and to record X-Volcano-CLI-Instruction / X-Volcano-CLI-Latest-Version / X-Volcano-Device-Instruction response headers via the new api.LastInstructions(). One process runs one command, so this is safe as process-global, mutex-guarded state. - internal/cmd/upgrade: removed the GitHub-polling notice machinery (MaybePrintUpdateNotice + on-disk cache) entirely; added PrintAPIInstructionNotices, which renders a non-blocking upgrade suggestion or a deprecation warning from api.LastInstructions() with zero extra network round-trips (it only reads headers the command's own API calls already received). `volcano upgrade`'s actual download/checksum/ cosign-verify flow (internal/update) is unchanged \u2014 GitHub remains the download source, only the "should I nudge/require upgrade" decision moves to the API. - cmd/volcano/main.go: prints notices unconditionally after Execute() (cobra skips PersistentPostRun on a RunE error, so this can't live there); a 426 (version_deprecation) error gets the concrete upgrade target appended when the API provided one; any other error gets a re-auth hint when the API signaled device_instruction=reauth. Security note: the claimed version has no cryptographic binding to a real release (volcano-cli is open source; per-version keypairs/checksums were considered and rejected \u2014 see volcano-hosting docs/cli/version-gating.md). This is intentional: the header is advisory input the CLI cooperates with, never a privilege, so spoofing it can only escape a restriction, never escalate one. Server-side counterpart: volcano-hosting#522 (merged after this), volcano-hosting#532 (dormant policy-arming staging PR). --- cmd/volcano/main.go | 46 ++++- cmd/volcano/main_test.go | 97 ++++++++++ internal/api/client.go | 7 + internal/api/client_test.go | 31 +++ internal/api/version_protocol.go | 99 ++++++++++ internal/api/version_protocol_test.go | 131 +++++++++++++ internal/cmd/root/root.go | 3 - internal/cmd/upgrade/upgrade.go | 195 ++++--------------- internal/cmd/upgrade/upgrade_test.go | 262 +++++--------------------- 9 files changed, 489 insertions(+), 382 deletions(-) create mode 100644 cmd/volcano/main_test.go create mode 100644 internal/api/version_protocol.go create mode 100644 internal/api/version_protocol_test.go diff --git a/cmd/volcano/main.go b/cmd/volcano/main.go index 0664f2e..6dbf50b 100644 --- a/cmd/volcano/main.go +++ b/cmd/volcano/main.go @@ -3,15 +3,57 @@ package main import ( "fmt" + "io" + "net/http" "os" + "github.com/Kong/volcano-cli/internal/api" rootcmd "github.com/Kong/volcano-cli/internal/cmd/root" + upgradecmd "github.com/Kong/volcano-cli/internal/cmd/upgrade" cliruntime "github.com/Kong/volcano-cli/internal/runtime" ) func main() { - if err := rootcmd.New(cliruntime.Deps{}).Execute(); err != nil { - fmt.Fprintln(os.Stderr, "Error:", err) + deps := cliruntime.Deps{} + root := rootcmd.New(deps) + err := root.Execute() + + if err != nil && api.Status(err) == http.StatusUpgradeRequired { + // The 426 body's message already reads "cli version no longer + // supported; run `volcano upgrade`"; just add the concrete upgrade + // target when the API provided one, and stop — printing the + // suggestion/deprecation notice below too would repeat ourselves. + printDeprecationError(os.Stderr, err, deps) + os.Exit(1) + } + + // Print any VOL-180 upgrade/deprecation notice unconditionally otherwise: + // covers the non-blocking suggestion, and a deprecated CLI succeeding on + // an exempt route (e.g. `login`) where the user still needs to know their + // CLI is deprecated even though this particular command was let through. + // Reads only in-process state (api.LastInstructions); adds no network call. + upgradecmd.PrintAPIInstructionNotices(root, deps) + + if err != nil { + printError(os.Stderr, err, deps) os.Exit(1) } } + +// printDeprecationError renders a version_deprecation 426 error together with +// the concrete upgrade target, when the API provided one. +func printDeprecationError(w io.Writer, err error, deps cliruntime.Deps) { + fmt.Fprintln(w, "Error:", err) + if latest := api.LastInstructions().LatestVersion; latest != "" { + fmt.Fprintf(w, "Upgrade to %s: %s\n", latest, cliruntime.CommandPath(deps, "upgrade")) + } +} + +// printError renders a generic command error, appending a reauth hint when +// the API signaled the platform token needs re-authentication (VOL-180). +func printError(w io.Writer, err error, deps cliruntime.Deps) { + fmt.Fprintln(w, "Error:", err) + if api.LastInstructions().DeviceInstruction == api.DeviceInstructionReauth { + fmt.Fprintf(w, "Run `%s` to re-authenticate.\n", cliruntime.CommandPath(deps, "login")) + } +} diff --git a/cmd/volcano/main_test.go b/cmd/volcano/main_test.go new file mode 100644 index 0000000..89d3d02 --- /dev/null +++ b/cmd/volcano/main_test.go @@ -0,0 +1,97 @@ +package main + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/Kong/volcano-cli/internal/api" + cliruntime "github.com/Kong/volcano-cli/internal/runtime" +) + +// withInstructions drives api.LastInstructions() through a real api.Client +// call against a test server, mirroring how production populates it from +// response headers (VOL-180). +func withInstructions(t *testing.T, latest, deviceInstruction string) { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if latest != "" { + w.Header().Set("X-Volcano-CLI-Latest-Version", latest) + } + if deviceInstruction != "" { + w.Header().Set("X-Volcano-Device-Instruction", deviceInstruction) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":[],"has_more":false,"page":1,"limit":100,"total":0}`)) + })) + t.Cleanup(server.Close) + + client, err := api.NewClient(server.URL, "", api.WithHTTPClient(server.Client())) + require.NoError(t, err) + _, err = client.ListProjects(context.Background(), api.DefaultPage, api.DefaultLimit) + require.NoError(t, err) +} + +func TestPrintDeprecationError_WithLatestVersion(t *testing.T) { + withInstructions(t, "v1.5.0", "") + var out bytes.Buffer + + printDeprecationError(&out, &api.Error{StatusCode: http.StatusUpgradeRequired, Message: "cli version no longer supported; run `volcano upgrade`"}, cliruntime.Deps{}) + + assert.Contains(t, out.String(), "Error: HTTP 426: cli version no longer supported; run `volcano upgrade`") + assert.Contains(t, out.String(), "Upgrade to v1.5.0: volcano upgrade") +} + +func TestPrintDeprecationError_WithoutLatestVersion(t *testing.T) { + withInstructions(t, "", "") + var out bytes.Buffer + + printDeprecationError(&out, &api.Error{StatusCode: http.StatusUpgradeRequired, Message: "cli version no longer supported"}, cliruntime.Deps{}) + + assert.Contains(t, out.String(), "Error: HTTP 426: cli version no longer supported") + assert.NotContains(t, out.String(), "Upgrade to") +} + +func TestPrintDeprecationError_UsesCommandPathPrefix(t *testing.T) { + withInstructions(t, "v1.5.0", "") + var out bytes.Buffer + + printDeprecationError(&out, &api.Error{StatusCode: http.StatusUpgradeRequired}, cliruntime.Deps{CommandPathPrefix: "acme"}) + + assert.Contains(t, out.String(), "Upgrade to v1.5.0: acme upgrade") +} + +func TestPrintError_ReauthHint(t *testing.T) { + withInstructions(t, "", "reauth") + var out bytes.Buffer + + printError(&out, &api.Error{StatusCode: http.StatusUnauthorized, Message: "token expired"}, cliruntime.Deps{}) + + assert.Contains(t, out.String(), "Error: HTTP 401: token expired") + assert.Contains(t, out.String(), "Run `volcano login` to re-authenticate.") +} + +func TestPrintError_NoReauthHintWithoutSignal(t *testing.T) { + withInstructions(t, "", "") + var out bytes.Buffer + + printError(&out, &api.Error{StatusCode: http.StatusInternalServerError, Message: "boom"}, cliruntime.Deps{}) + + assert.Contains(t, out.String(), "Error: HTTP 500: boom") + assert.NotContains(t, out.String(), "re-authenticate") +} + +func TestPrintError_ReauthHintUsesCommandPathPrefix(t *testing.T) { + withInstructions(t, "", "reauth") + var out bytes.Buffer + + printError(&out, &api.Error{StatusCode: http.StatusUnauthorized}, cliruntime.Deps{CommandPathPrefix: "acme"}) + + assert.Contains(t, out.String(), "Run `acme login` to re-authenticate.") +} diff --git a/internal/api/client.go b/internal/api/client.go index 07f25f8..25f1056 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -70,6 +70,13 @@ func NewClient(apiURL, token string, opts ...Option) (*Client, error) { cfg.streamHTTPClient = &http.Client{} } + // Wrap both doers with the VOL-180 version protocol: every request + // (including unauthenticated ones, e.g. device/token exchange) reports this + // CLI's version/identity, and every response's instruction headers are + // recorded for LastInstructions regardless of which call observed them. + cfg.httpClient = versionProtocolDoer{next: cfg.httpClient} + cfg.streamHTTPClient = versionProtocolDoer{next: cfg.streamHTTPClient} + baseURL := generatedClientBaseURL(parsed) clientOpts := []apiclient.ClientOption{ apiclient.WithHTTPClient(cfg.httpClient), diff --git a/internal/api/client_test.go b/internal/api/client_test.go index 3cd3a9a..4e150d1 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/Kong/volcano-cli/internal/apiclient" + "github.com/Kong/volcano-cli/internal/version" ) func TestPollDeviceTokenUnexpectedStatusReturnsError(t *testing.T) { @@ -108,6 +109,36 @@ func TestWebSignupURLRejectsBadInput(t *testing.T) { } } +func TestNewClientSendsVersionProtocolHeadersAndRecordsInstructions(t *testing.T) { + resetInstructions(t) + oldVersion := version.Version + version.Version = "v1.2.3" + t.Cleanup(func() { version.Version = oldVersion }) + + var sawVersionHeader, sawUA string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sawVersionHeader = r.Header.Get("X-Volcano-CLI-Version") + sawUA = r.Header.Get("User-Agent") + w.Header().Set("X-Volcano-CLI-Instruction", CLIInstructionSuggestionUpgrade) + w.Header().Set("X-Volcano-CLI-Latest-Version", "v1.5.0") + writeAPIJSON(t, w, http.StatusOK, map[string]any{"data": []any{}, "has_more": false, "page": 1, "limit": 100, "total": 0}) + })) + defer server.Close() + + client, err := NewClient(server.URL, "token", WithHTTPClient(server.Client())) + require.NoError(t, err) + + _, err = client.ListProjects(context.Background(), DefaultPage, DefaultLimit) + require.NoError(t, err) + + assert.Equal(t, "v1.2.3", sawVersionHeader) + assert.Contains(t, sawUA, "volcano-cli/v1.2.3") + + got := LastInstructions() + assert.Equal(t, CLIInstructionSuggestionUpgrade, got.CLIInstruction) + assert.Equal(t, "v1.5.0", got.LatestVersion) +} + func TestNewClientPreservesAPIURLPathPrefix(t *testing.T) { var sawPath string var sawQuery string diff --git a/internal/api/version_protocol.go b/internal/api/version_protocol.go new file mode 100644 index 0000000..cc62595 --- /dev/null +++ b/internal/api/version_protocol.go @@ -0,0 +1,99 @@ +package api + +import ( + "net/http" + "runtime" + "strings" + "sync" + + "github.com/Kong/volcano-cli/internal/apiclient" + "github.com/Kong/volcano-cli/internal/version" +) + +// Header names and instruction values for the VOL-180 CLI version protocol. +// These MUST match volcano-hosting's internal/constants/http.go and +// docs/cli/version-gating.md. Duplicated here because this is a separate +// repository with no shared Go module; there is intentionally no +// cryptographic binding on the request header — see "Security model" in that +// doc for why the claimed version is advisory input only and never a +// privilege (the CLI cannot make this header trustworthy, and doesn't need +// to: the API never grants anything based on it). +const ( + headerCLIVersion = "X-Volcano-CLI-Version" + headerCLIInstruction = "X-Volcano-CLI-Instruction" + headerCLILatestVersion = "X-Volcano-CLI-Latest-Version" + headerDeviceInstruction = "X-Volcano-Device-Instruction" + + // CLIInstructionSuggestionUpgrade signals a newer CLI is published; non-blocking. + CLIInstructionSuggestionUpgrade = "suggestion_upgrade" + // CLIInstructionVersionDeprecation signals this CLI version is no longer + // supported. The API pairs it with an HTTP 426 on non-exempt routes. + CLIInstructionVersionDeprecation = "version_deprecation" + // DeviceInstructionReauth signals the platform token needs re-authentication. + DeviceInstructionReauth = "reauth" +) + +// Instructions is a snapshot of the most recent VOL-180 protocol signals +// observed on any API response in this process. +type Instructions struct { + CLIInstruction string + LatestVersion string + DeviceInstruction string +} + +var ( + instructionsMu sync.RWMutex + lastInstructions Instructions +) + +// LastInstructions returns the most recently observed VOL-180 instructions +// from any API response in this process. It is the zero value if no API call +// has completed yet — e.g. local-only commands (init, help, version, +// completion) never populate this, since they never make a request. +// +// A CLI process runs exactly one command per invocation, so "most recent" is +// simply "from the request(s) this command made" — there is no cross-command +// state to reconcile. +func LastInstructions() Instructions { + instructionsMu.RLock() + defer instructionsMu.RUnlock() + return lastInstructions +} + +func recordInstructions(header http.Header) { + next := Instructions{ + CLIInstruction: strings.TrimSpace(header.Get(headerCLIInstruction)), + LatestVersion: strings.TrimSpace(header.Get(headerCLILatestVersion)), + DeviceInstruction: strings.TrimSpace(header.Get(headerDeviceInstruction)), + } + instructionsMu.Lock() + lastInstructions = next + instructionsMu.Unlock() +} + +// userAgent identifies this CLI build to the API — for support/debugging and +// as the VOL-180 pre-protocol/attribution signal — replacing the Go HTTP +// client's default user agent. +func userAgent() string { + return "volcano-cli/" + version.Version + " (" + runtime.GOOS + "/" + runtime.GOARCH + ")" +} + +// versionProtocolDoer wraps an apiclient.HttpRequestDoer to speak the VOL-180 +// CLI version protocol on every request: it reports this CLI's version and +// identity, and records whatever instructions the API returns so callers can +// act on them via LastInstructions. It never alters the request/response +// beyond adding headers — bodies and errors pass through untouched, and it +// never retries. +type versionProtocolDoer struct { + next apiclient.HttpRequestDoer +} + +func (d versionProtocolDoer) Do(req *http.Request) (*http.Response, error) { + req.Header.Set(headerCLIVersion, version.Version) + req.Header.Set("User-Agent", userAgent()) + resp, err := d.next.Do(req) + if resp != nil { + recordInstructions(resp.Header) + } + return resp, err +} diff --git a/internal/api/version_protocol_test.go b/internal/api/version_protocol_test.go new file mode 100644 index 0000000..fdabcef --- /dev/null +++ b/internal/api/version_protocol_test.go @@ -0,0 +1,131 @@ +package api + +import ( + "net/http" + "net/http/httptest" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/Kong/volcano-cli/internal/version" +) + +// resetInstructions clears process-global observed instructions between test +// cases (production never needs this: each CLI invocation is a fresh process). +func resetInstructions(t *testing.T) { + t.Helper() + instructionsMu.Lock() + lastInstructions = Instructions{} + instructionsMu.Unlock() + t.Cleanup(func() { + instructionsMu.Lock() + lastInstructions = Instructions{} + instructionsMu.Unlock() + }) +} + +func TestVersionProtocolDoer_SetsVersionHeaderAndUserAgent(t *testing.T) { + resetInstructions(t) + oldVersion := version.Version + version.Version = "v1.4.2" + t.Cleanup(func() { version.Version = oldVersion }) + + var gotVersionHeader, gotUA string + inner := doerFunc(func(req *http.Request) (*http.Response, error) { + gotVersionHeader = req.Header.Get(headerCLIVersion) + gotUA = req.Header.Get("User-Agent") + return httptest.NewRecorder().Result(), nil + }) + + req := httptest.NewRequest(http.MethodGet, "/projects", http.NoBody) + _, err := versionProtocolDoer{next: inner}.Do(req) + require.NoError(t, err) + + assert.Equal(t, "v1.4.2", gotVersionHeader) + assert.Equal(t, "volcano-cli/v1.4.2 ("+runtime.GOOS+"/"+runtime.GOARCH+")", gotUA) +} + +func TestVersionProtocolDoer_RecordsInstructionHeaders(t *testing.T) { + resetInstructions(t) + + inner := doerFunc(func(*http.Request) (*http.Response, error) { + rec := httptest.NewRecorder() + rec.Header().Set(headerCLIInstruction, CLIInstructionSuggestionUpgrade) + rec.Header().Set(headerCLILatestVersion, "v1.5.0") + rec.WriteHeader(http.StatusOK) + return rec.Result(), nil + }) + + req := httptest.NewRequest(http.MethodGet, "/projects", http.NoBody) + _, err := versionProtocolDoer{next: inner}.Do(req) + require.NoError(t, err) + + got := LastInstructions() + assert.Equal(t, CLIInstructionSuggestionUpgrade, got.CLIInstruction) + assert.Equal(t, "v1.5.0", got.LatestVersion) + assert.Empty(t, got.DeviceInstruction) +} + +func TestVersionProtocolDoer_RecordsReauthInstruction(t *testing.T) { + resetInstructions(t) + + inner := doerFunc(func(*http.Request) (*http.Response, error) { + rec := httptest.NewRecorder() + rec.Header().Set(headerDeviceInstruction, DeviceInstructionReauth) + rec.WriteHeader(http.StatusUnauthorized) + return rec.Result(), nil + }) + + req := httptest.NewRequest(http.MethodGet, "/projects", http.NoBody) + _, err := versionProtocolDoer{next: inner}.Do(req) + require.NoError(t, err) + + assert.Equal(t, DeviceInstructionReauth, LastInstructions().DeviceInstruction) +} + +func TestVersionProtocolDoer_NoResponseIsNoOp(t *testing.T) { + resetInstructions(t) + + boom := assert.AnError + inner := doerFunc(func(*http.Request) (*http.Response, error) { + return nil, boom + }) + + req := httptest.NewRequest(http.MethodGet, "/projects", http.NoBody) + _, err := versionProtocolDoer{next: inner}.Do(req) + require.ErrorIs(t, err, boom) + assert.Equal(t, Instructions{}, LastInstructions()) +} + +func TestVersionProtocolDoer_LatestInvocationWins(t *testing.T) { + // A CLI process runs one command per invocation; "most recent" reflects the + // last response observed within that single run. + resetInstructions(t) + + first := doerFunc(func(*http.Request) (*http.Response, error) { + rec := httptest.NewRecorder() + rec.Header().Set(headerCLIInstruction, CLIInstructionSuggestionUpgrade) + rec.WriteHeader(http.StatusOK) + return rec.Result(), nil + }) + second := doerFunc(func(*http.Request) (*http.Response, error) { + rec := httptest.NewRecorder() + rec.WriteHeader(http.StatusOK) + return rec.Result(), nil + }) + + req := httptest.NewRequest(http.MethodGet, "/projects", http.NoBody) + _, err := versionProtocolDoer{next: first}.Do(req) + require.NoError(t, err) + require.Equal(t, CLIInstructionSuggestionUpgrade, LastInstructions().CLIInstruction) + + _, err = versionProtocolDoer{next: second}.Do(req) + require.NoError(t, err) + assert.Empty(t, LastInstructions().CLIInstruction, "a later response with no instruction header clears the prior one") +} + +type doerFunc func(*http.Request) (*http.Response, error) + +func (f doerFunc) Do(req *http.Request) (*http.Response, error) { return f(req) } diff --git a/internal/cmd/root/root.go b/internal/cmd/root/root.go index 936e119..e6c09cd 100644 --- a/internal/cmd/root/root.go +++ b/internal/cmd/root/root.go @@ -27,9 +27,6 @@ func New(deps cliruntime.Deps) *cobra.Command { Long: "volcano is the command-line client for the Volcano hosting platform.", SilenceUsage: true, SilenceErrors: true, - PersistentPreRun: func(cmd *cobra.Command, _ []string) { - upgradecmd.MaybePrintUpdateNotice(cmd, deps) - }, RunE: func(cmd *cobra.Command, _ []string) error { if showVersion { printVersion(cmd.OutOrStdout()) diff --git a/internal/cmd/upgrade/upgrade.go b/internal/cmd/upgrade/upgrade.go index c90ecf6..b3acd40 100644 --- a/internal/cmd/upgrade/upgrade.go +++ b/internal/cmd/upgrade/upgrade.go @@ -2,31 +2,16 @@ package upgrade import ( - "context" - "encoding/json" "fmt" - "os" - "path/filepath" - "strings" - "time" "github.com/spf13/cobra" + "github.com/Kong/volcano-cli/internal/api" cliruntime "github.com/Kong/volcano-cli/internal/runtime" "github.com/Kong/volcano-cli/internal/update" "github.com/Kong/volcano-cli/internal/version" ) -const ( - updateCheckTimeout = 2 * time.Second - updateCheckMaxAge = 24 * time.Hour -) - -type noticeCache struct { - CheckedAt time.Time `json:"checked_at"` - Latest string `json:"latest"` -} - // New returns the upgrade command. func New(deps cliruntime.Deps) *cobra.Command { var verifySignature bool @@ -53,156 +38,46 @@ func updateOptions(deps cliruntime.Deps) update.Options { } } -// MaybePrintUpdateNotice checks whether a newer CLI release is available and prints a non-blocking notice. -func MaybePrintUpdateNotice(cmd *cobra.Command, deps cliruntime.Deps) { - if shouldSkipUpdateCheck(cmd) { - return - } - if notice, ok := noticeFromCache(version.Version); ok { - if notice != nil { - printUpdateNotice(cmd, notice) - } - return - } - parent := cmd.Context() - if parent == nil { - parent = context.Background() - } - ctx, cancel := context.WithTimeout(parent, updateCheckTimeout) - defer cancel() - release, err := update.LatestRelease(ctx, updateOptions(deps)) - if err != nil { - return - } - latest := strings.TrimSpace(release.TagName) - if latest == "" { - return - } - writeNoticeCache(latest) - newer, err := update.NewerThan(latest, version.Version) - if err != nil || !newer { - return - } - notice := &update.Notice{Current: version.Version, Latest: latest} - printUpdateNotice(cmd, notice) -} - -func shouldSkipUpdateCheck(cmd *cobra.Command) bool { - if version.Version == "dev" { - return true - } - if cmd.Flags().Changed("help") || cmd.Flags().Changed("version") { - return true - } - // Skip when invoking the root command with no subcommand — cobra renders help, and - // we don't want a network check to delay or pollute that output. - if cmd.Parent() == nil && len(cmd.Flags().Args()) == 0 { - return true - } - for c := cmd; c != nil; c = c.Parent() { - if skipUpdateCheckCommand(c.Name()) { - return true - } - } - return false -} - -func skipUpdateCheckCommand(name string) bool { - switch name { - case "version", "upgrade", "help", "completion", "__complete", "__completeNoDesc": - return true - default: - return false - } -} - -func printUpdateNotice(cmd *cobra.Command, notice *update.Notice) { - fmt.Fprintf(cmd.ErrOrStderr(), "A newer Volcano CLI version is available: %s (current %s). Run `volcano upgrade` to upgrade.\n", notice.Latest, notice.Current) -} - -func noticeFromCache(current string) (*update.Notice, bool) { - cache, err := readNoticeCache() - if err != nil || cache.Latest == "" { - return nil, false - } - // Use absolute age so future-dated CheckedAt values (clock skew, restored backups) - // are treated as stale rather than fresh-forever. - age := time.Since(cache.CheckedAt) - if age < 0 { - age = -age - } - if age > updateCheckMaxAge { - return nil, false - } - newer, err := update.NewerThan(cache.Latest, current) - if err != nil { - return nil, false - } - if !newer { - return nil, true +// PrintAPIInstructionNotices prints a non-blocking upgrade suggestion or a +// deprecation warning based on the most recent VOL-180 instructions observed +// on this invocation's API responses (api.LastInstructions). This replaces +// the previous GitHub-release polling notice (VOL-168): the API is now the +// sole source of truth for whether/how loudly to nudge an upgrade, and the +// check adds no extra network round-trip — it only reads headers the command +// already received. +// +// A command that made no API call (help, version, completion, local-only +// commands) observes a zero-value Instructions and prints nothing. +func PrintAPIInstructionNotices(cmd *cobra.Command, deps cliruntime.Deps) { + instructions := api.LastInstructions() + switch instructions.CLIInstruction { + case api.CLIInstructionVersionDeprecation: + printDeprecationWarning(cmd, deps, instructions.LatestVersion) + case api.CLIInstructionSuggestionUpgrade: + printUpgradeSuggestion(cmd, deps, instructions.LatestVersion) } - return &update.Notice{Current: current, Latest: cache.Latest}, true } -func readNoticeCache() (*noticeCache, error) { - path, err := noticeCachePath() - if err != nil { - return nil, err - } - f, err := os.Open(path) - if err != nil { - return nil, err - } - defer func() { _ = f.Close() }() - var cache noticeCache - if err := json.NewDecoder(f).Decode(&cache); err != nil { - return nil, err - } - return &cache, nil -} - -func writeNoticeCache(latest string) { - path, err := noticeCachePath() - if err != nil { +func printUpgradeSuggestion(cmd *cobra.Command, deps cliruntime.Deps, latest string) { + upgradeCmd := cliruntime.CommandPath(deps, "upgrade") + if latest != "" { + fmt.Fprintf(cmd.ErrOrStderr(), "A newer Volcano CLI version is available: %s (current %s). Run `%s` to upgrade.\n", latest, version.Version, upgradeCmd) return } - dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0o700); err != nil { - return - } - // Write to a sibling temp file and rename, so concurrent CLI invocations can't - // observe a half-written JSON file. - f, err := os.CreateTemp(dir, "update-check-*.json") - if err != nil { - return - } - tmpPath := f.Name() - cleanup := true - defer func() { - if cleanup { - _ = os.Remove(tmpPath) - } - }() - if err := json.NewEncoder(f).Encode(noticeCache{CheckedAt: time.Now().UTC(), Latest: latest}); err != nil { - _ = f.Close() - return - } - if err := f.Close(); err != nil { - return - } - if err := os.Chmod(tmpPath, 0o600); err != nil { - return - } - if err := os.Rename(tmpPath, path); err != nil { - return - } - cleanup = false + fmt.Fprintf(cmd.ErrOrStderr(), "A newer Volcano CLI version is available (current %s). Run `%s` to upgrade.\n", version.Version, upgradeCmd) } -func noticeCachePath() (string, error) { - dir, err := os.UserCacheDir() - if err != nil { - return "", err +// printDeprecationWarning prints a hard warning for a version the API has +// deprecated. Gated routes already 426 this request (see api.Status(err) == +// http.StatusUpgradeRequired handling in cmd/volcano/main.go); this notice +// also fires on exempt routes (e.g. a deprecated CLI's successful `volcano +// login`), so the user learns their CLI is deprecated even when the specific +// command they ran was allowed through. +func printDeprecationWarning(cmd *cobra.Command, deps cliruntime.Deps, latest string) { + upgradeCmd := cliruntime.CommandPath(deps, "upgrade") + if latest != "" { + fmt.Fprintf(cmd.ErrOrStderr(), "Volcano CLI %s is no longer supported. Upgrade to %s or later:\n %s\n", version.Version, latest, upgradeCmd) + return } - return filepath.Join(dir, "volcano", "update-check.json"), nil + fmt.Fprintf(cmd.ErrOrStderr(), "Volcano CLI %s is no longer supported. Run `%s` to upgrade.\n", version.Version, upgradeCmd) } diff --git a/internal/cmd/upgrade/upgrade_test.go b/internal/cmd/upgrade/upgrade_test.go index 40b7791..a616f92 100644 --- a/internal/cmd/upgrade/upgrade_test.go +++ b/internal/cmd/upgrade/upgrade_test.go @@ -12,12 +12,12 @@ import ( "os" "path/filepath" "testing" - "time" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/Kong/volcano-cli/internal/api" cliruntime "github.com/Kong/volcano-cli/internal/runtime" "github.com/Kong/volcano-cli/internal/update" "github.com/Kong/volcano-cli/internal/version" @@ -100,231 +100,85 @@ func TestUpgradeCommandRejectsArgs(t *testing.T) { assert.ErrorContains(t, err, `unknown command "v1.2.3"`) } -func TestUpdateNoticePrintsForOutdatedVersion(t *testing.T) { - setUpgradeTestCacheDir(t) - oldVersion := version.Version - version.Version = "v1.2.3" - t.Cleanup(func() { version.Version = oldVersion }) - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "/releases/latest", r.URL.Path) - writeUpgradeJSON(t, w, update.Release{TagName: "v1.2.4"}) - })) - defer server.Close() - - cmd := noticeTestCommand("init") - var out bytes.Buffer - cmd.SetErr(&out) - MaybePrintUpdateNotice(cmd, cliruntime.Deps{ - HTTPClient: server.Client(), - UpdateGitHubAPIURL: server.URL, - }) - assert.Contains(t, out.String(), "A newer Volcano CLI version is available: v1.2.4 (current v1.2.3). Run `volcano upgrade` to upgrade.") -} - -func TestUpdateNoticeSkipsVersionCommand(t *testing.T) { - setUpgradeTestCacheDir(t) - oldVersion := version.Version - version.Version = "v1.2.3" - t.Cleanup(func() { version.Version = oldVersion }) - - requests := 0 +// withInstructions drives api.LastInstructions() through a real api.Client +// call against a test server, exercising PrintAPIInstructionNotices exactly +// as production does (via observed response headers), with no reliance on +// api's unexported state. +func withInstructions(t *testing.T, cliInstruction, latest, deviceInstruction string) { + t.Helper() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - requests++ - writeUpgradeJSON(t, w, update.Release{TagName: "v1.2.4"}) + if cliInstruction != "" { + w.Header().Set("X-Volcano-CLI-Instruction", cliInstruction) + } + if latest != "" { + w.Header().Set("X-Volcano-CLI-Latest-Version", latest) + } + if deviceInstruction != "" { + w.Header().Set("X-Volcano-Device-Instruction", deviceInstruction) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":[],"has_more":false,"page":1,"limit":100,"total":0}`)) })) - defer server.Close() + t.Cleanup(server.Close) - cmd := noticeTestCommand("version") - var out bytes.Buffer - cmd.SetErr(&out) - MaybePrintUpdateNotice(cmd, cliruntime.Deps{ - HTTPClient: server.Client(), - UpdateGitHubAPIURL: server.URL, - }) - assert.Empty(t, out.String()) - assert.Zero(t, requests) + client, err := api.NewClient(server.URL, "", api.WithHTTPClient(server.Client())) + require.NoError(t, err) + _, err = client.ListProjects(context.Background(), api.DefaultPage, api.DefaultLimit) + require.NoError(t, err) } -func TestUpdateNoticeSkipsImplicitRootHelp(t *testing.T) { - setUpgradeTestCacheDir(t) +func TestPrintAPIInstructionNotices_Suggestion(t *testing.T) { oldVersion := version.Version version.Version = "v1.2.3" t.Cleanup(func() { version.Version = oldVersion }) + withInstructions(t, api.CLIInstructionSuggestionUpgrade, "v1.5.0", "") - requests := 0 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - requests++ - writeUpgradeJSON(t, w, update.Release{TagName: "v1.2.4"}) - })) - defer server.Close() - - cmd := &cobra.Command{Use: "volcano"} + cmd := &cobra.Command{Use: "projects"} var out bytes.Buffer cmd.SetErr(&out) - MaybePrintUpdateNotice(cmd, cliruntime.Deps{ - HTTPClient: server.Client(), - UpdateGitHubAPIURL: server.URL, - }) - assert.Empty(t, out.String()) - assert.Zero(t, requests) -} - -func TestUpdateNoticeSkipsCompletionCommands(t *testing.T) { - setUpgradeTestCacheDir(t) - oldVersion := version.Version - version.Version = "v1.2.3" - t.Cleanup(func() { version.Version = oldVersion }) + PrintAPIInstructionNotices(cmd, cliruntime.Deps{}) - for _, name := range []string{"completion", "__complete", "__completeNoDesc"} { - t.Run(name, func(t *testing.T) { - requests := 0 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - requests++ - writeUpgradeJSON(t, w, update.Release{TagName: "v1.2.4"}) - })) - defer server.Close() - - cmd := noticeTestCommand(name) - var out bytes.Buffer - cmd.SetErr(&out) - MaybePrintUpdateNotice(cmd, cliruntime.Deps{ - HTTPClient: server.Client(), - UpdateGitHubAPIURL: server.URL, - }) - assert.Empty(t, out.String()) - assert.Zero(t, requests) - }) - } + assert.Contains(t, out.String(), "A newer Volcano CLI version is available: v1.5.0 (current v1.2.3). Run `volcano upgrade` to upgrade.") } -func TestUpdateNoticeUsesFreshCache(t *testing.T) { - setUpgradeTestCacheDir(t) +func TestPrintAPIInstructionNotices_SuggestionWithoutLatestVersion(t *testing.T) { oldVersion := version.Version version.Version = "v1.2.3" t.Cleanup(func() { version.Version = oldVersion }) - writeUpgradeTestCache(t, "v1.2.4", time.Now().UTC()) - - requests := 0 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - requests++ - writeUpgradeJSON(t, w, update.Release{TagName: "v1.2.5"}) - })) - defer server.Close() + withInstructions(t, api.CLIInstructionSuggestionUpgrade, "", "") - cmd := noticeTestCommand("init") + cmd := &cobra.Command{Use: "projects"} var out bytes.Buffer cmd.SetErr(&out) - MaybePrintUpdateNotice(cmd, cliruntime.Deps{ - HTTPClient: server.Client(), - UpdateGitHubAPIURL: server.URL, - }) - assert.Contains(t, out.String(), "v1.2.4") - assert.NotContains(t, out.String(), "v1.2.5") - assert.Zero(t, requests) -} + PrintAPIInstructionNotices(cmd, cliruntime.Deps{}) -func TestUpdateNoticeUsesFreshUpToDateCache(t *testing.T) { - setUpgradeTestCacheDir(t) - oldVersion := version.Version - version.Version = "v1.2.4" - t.Cleanup(func() { version.Version = oldVersion }) - writeUpgradeTestCache(t, "v1.2.4", time.Now().UTC()) - - requests := 0 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - requests++ - writeUpgradeJSON(t, w, update.Release{TagName: "v1.2.5"}) - })) - defer server.Close() - - cmd := noticeTestCommand("init") - var out bytes.Buffer - cmd.SetErr(&out) - MaybePrintUpdateNotice(cmd, cliruntime.Deps{ - HTTPClient: server.Client(), - UpdateGitHubAPIURL: server.URL, - }) - assert.Empty(t, out.String()) - assert.Zero(t, requests) + assert.Contains(t, out.String(), "A newer Volcano CLI version is available (current v1.2.3). Run `volcano upgrade` to upgrade.") } -func TestUpdateNoticeWritesCacheWhenAlreadyUpToDate(t *testing.T) { - setUpgradeTestCacheDir(t) +func TestPrintAPIInstructionNotices_Deprecation(t *testing.T) { oldVersion := version.Version - version.Version = "v1.2.4" + version.Version = "v0.9.0" t.Cleanup(func() { version.Version = oldVersion }) + withInstructions(t, api.CLIInstructionVersionDeprecation, "v1.5.0", "") - requests := 0 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - requests++ - writeUpgradeJSON(t, w, update.Release{TagName: "v1.2.4"}) - })) - defer server.Close() - - cmd := noticeTestCommand("init") + cmd := &cobra.Command{Use: "login"} var out bytes.Buffer cmd.SetErr(&out) - deps := cliruntime.Deps{HTTPClient: server.Client(), UpdateGitHubAPIURL: server.URL} - MaybePrintUpdateNotice(cmd, deps) - assert.Empty(t, out.String()) - assert.Equal(t, 1, requests) + PrintAPIInstructionNotices(cmd, cliruntime.Deps{}) - MaybePrintUpdateNotice(cmd, deps) - assert.Empty(t, out.String()) - assert.Equal(t, 1, requests) + assert.Contains(t, out.String(), "Volcano CLI v0.9.0 is no longer supported. Upgrade to v1.5.0 or later:\n volcano upgrade") } -func TestUpdateNoticeRejectsFutureDatedCache(t *testing.T) { - setUpgradeTestCacheDir(t) - oldVersion := version.Version - version.Version = "v1.2.3" - t.Cleanup(func() { version.Version = oldVersion }) - // A future CheckedAt (clock skew, restored backup) must not be trusted as fresh — - // the notice check should re-fetch from the network instead. - writeUpgradeTestCache(t, "v1.2.4", time.Now().Add(48*time.Hour).UTC()) - - requests := 0 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - requests++ - writeUpgradeJSON(t, w, update.Release{TagName: "v1.2.5"}) - })) - defer server.Close() +func TestPrintAPIInstructionNotices_UsesCommandPathPrefix(t *testing.T) { + withInstructions(t, api.CLIInstructionSuggestionUpgrade, "v1.5.0", "") - cmd := noticeTestCommand("init") + cmd := &cobra.Command{Use: "projects"} var out bytes.Buffer cmd.SetErr(&out) - MaybePrintUpdateNotice(cmd, cliruntime.Deps{ - HTTPClient: server.Client(), - UpdateGitHubAPIURL: server.URL, - }) - assert.Contains(t, out.String(), "v1.2.5") - assert.Equal(t, 1, requests) -} - -func TestUpdateNoticeRefreshesStaleCache(t *testing.T) { - setUpgradeTestCacheDir(t) - oldVersion := version.Version - version.Version = "v1.2.3" - t.Cleanup(func() { version.Version = oldVersion }) - writeUpgradeTestCache(t, "v1.2.4", time.Now().Add(-25*time.Hour).UTC()) - - requests := 0 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - requests++ - writeUpgradeJSON(t, w, update.Release{TagName: "v1.2.5"}) - })) - defer server.Close() + PrintAPIInstructionNotices(cmd, cliruntime.Deps{CommandPathPrefix: "acme"}) - cmd := noticeTestCommand("init") - var out bytes.Buffer - cmd.SetErr(&out) - MaybePrintUpdateNotice(cmd, cliruntime.Deps{ - HTTPClient: server.Client(), - UpdateGitHubAPIURL: server.URL, - }) - assert.Contains(t, out.String(), "v1.2.5") - assert.Equal(t, 1, requests) + assert.Contains(t, out.String(), "Run `acme upgrade` to upgrade.") } func executeUpgradeCommand(t *testing.T, cmd *cobra.Command, args ...string) (string, error) { @@ -337,13 +191,6 @@ func executeUpgradeCommand(t *testing.T, cmd *cobra.Command, args ...string) (st return out.String(), err } -func noticeTestCommand(name string) *cobra.Command { - root := &cobra.Command{Use: "volcano"} - cmd := &cobra.Command{Use: name} - root.AddCommand(cmd) - return cmd -} - func newUpgradeTestServer(t *testing.T, binaryName string, binary []byte, checksums string) *httptest.Server { t.Helper() return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -371,22 +218,3 @@ func writeUpgradeJSON(t *testing.T, w http.ResponseWriter, value any) { w.Header().Set("Content-Type", "application/json") require.NoError(t, json.NewEncoder(w).Encode(value)) } - -func setUpgradeTestCacheDir(t *testing.T) string { - t.Helper() - dir := t.TempDir() - t.Setenv("HOME", dir) - t.Setenv("XDG_CACHE_HOME", filepath.Join(dir, ".cache")) - return dir -} - -func writeUpgradeTestCache(t *testing.T, latest string, checkedAt time.Time) { - t.Helper() - path, err := noticeCachePath() - require.NoError(t, err) - require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) - f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) - require.NoError(t, err) - defer func() { require.NoError(t, f.Close()) }() - require.NoError(t, json.NewEncoder(f).Encode(noticeCache{CheckedAt: checkedAt, Latest: latest})) -} From 4ade6e82ac4015fb0abe1660f0d53815a6156606 Mon Sep 17 00:00:00 2001 From: Ted Kim Date: Wed, 8 Jul 2026 15:54:21 -0400 Subject: [PATCH 2/6] feat: rename version instructions for clarity (VOL-180) Rename to leave room for other instruction categories: - suggestion_upgrade -> suggestion_version_upgrade - version_deprecation -> require_version_upgrade Constant names follow: CLIInstructionSuggestionVersionUpgrade, CLIInstructionRequireVersionUpgrade. Server counterpart: volcano-hosting#522. --- cmd/volcano/main.go | 2 +- internal/api/client_test.go | 4 ++-- internal/api/version_protocol.go | 8 ++++---- internal/api/version_protocol_test.go | 8 ++++---- internal/cmd/upgrade/upgrade.go | 4 ++-- internal/cmd/upgrade/upgrade_test.go | 8 ++++---- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/cmd/volcano/main.go b/cmd/volcano/main.go index 6dbf50b..ece608f 100644 --- a/cmd/volcano/main.go +++ b/cmd/volcano/main.go @@ -40,7 +40,7 @@ func main() { } } -// printDeprecationError renders a version_deprecation 426 error together with +// printDeprecationError renders a require_version_upgrade 426 error together with // the concrete upgrade target, when the API provided one. func printDeprecationError(w io.Writer, err error, deps cliruntime.Deps) { fmt.Fprintln(w, "Error:", err) diff --git a/internal/api/client_test.go b/internal/api/client_test.go index 4e150d1..95b5cbd 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -119,7 +119,7 @@ func TestNewClientSendsVersionProtocolHeadersAndRecordsInstructions(t *testing.T server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { sawVersionHeader = r.Header.Get("X-Volcano-CLI-Version") sawUA = r.Header.Get("User-Agent") - w.Header().Set("X-Volcano-CLI-Instruction", CLIInstructionSuggestionUpgrade) + w.Header().Set("X-Volcano-CLI-Instruction", CLIInstructionSuggestionVersionUpgrade) w.Header().Set("X-Volcano-CLI-Latest-Version", "v1.5.0") writeAPIJSON(t, w, http.StatusOK, map[string]any{"data": []any{}, "has_more": false, "page": 1, "limit": 100, "total": 0}) })) @@ -135,7 +135,7 @@ func TestNewClientSendsVersionProtocolHeadersAndRecordsInstructions(t *testing.T assert.Contains(t, sawUA, "volcano-cli/v1.2.3") got := LastInstructions() - assert.Equal(t, CLIInstructionSuggestionUpgrade, got.CLIInstruction) + assert.Equal(t, CLIInstructionSuggestionVersionUpgrade, got.CLIInstruction) assert.Equal(t, "v1.5.0", got.LatestVersion) } diff --git a/internal/api/version_protocol.go b/internal/api/version_protocol.go index cc62595..a26c43a 100644 --- a/internal/api/version_protocol.go +++ b/internal/api/version_protocol.go @@ -24,11 +24,11 @@ const ( headerCLILatestVersion = "X-Volcano-CLI-Latest-Version" headerDeviceInstruction = "X-Volcano-Device-Instruction" - // CLIInstructionSuggestionUpgrade signals a newer CLI is published; non-blocking. - CLIInstructionSuggestionUpgrade = "suggestion_upgrade" - // CLIInstructionVersionDeprecation signals this CLI version is no longer + // CLIInstructionSuggestionVersionUpgrade signals a newer CLI is published; non-blocking. + CLIInstructionSuggestionVersionUpgrade = "suggestion_version_upgrade" + // CLIInstructionRequireVersionUpgrade signals this CLI version is no longer // supported. The API pairs it with an HTTP 426 on non-exempt routes. - CLIInstructionVersionDeprecation = "version_deprecation" + CLIInstructionRequireVersionUpgrade = "require_version_upgrade" // DeviceInstructionReauth signals the platform token needs re-authentication. DeviceInstructionReauth = "reauth" ) diff --git a/internal/api/version_protocol_test.go b/internal/api/version_protocol_test.go index fdabcef..ab2277b 100644 --- a/internal/api/version_protocol_test.go +++ b/internal/api/version_protocol_test.go @@ -52,7 +52,7 @@ func TestVersionProtocolDoer_RecordsInstructionHeaders(t *testing.T) { inner := doerFunc(func(*http.Request) (*http.Response, error) { rec := httptest.NewRecorder() - rec.Header().Set(headerCLIInstruction, CLIInstructionSuggestionUpgrade) + rec.Header().Set(headerCLIInstruction, CLIInstructionSuggestionVersionUpgrade) rec.Header().Set(headerCLILatestVersion, "v1.5.0") rec.WriteHeader(http.StatusOK) return rec.Result(), nil @@ -63,7 +63,7 @@ func TestVersionProtocolDoer_RecordsInstructionHeaders(t *testing.T) { require.NoError(t, err) got := LastInstructions() - assert.Equal(t, CLIInstructionSuggestionUpgrade, got.CLIInstruction) + assert.Equal(t, CLIInstructionSuggestionVersionUpgrade, got.CLIInstruction) assert.Equal(t, "v1.5.0", got.LatestVersion) assert.Empty(t, got.DeviceInstruction) } @@ -106,7 +106,7 @@ func TestVersionProtocolDoer_LatestInvocationWins(t *testing.T) { first := doerFunc(func(*http.Request) (*http.Response, error) { rec := httptest.NewRecorder() - rec.Header().Set(headerCLIInstruction, CLIInstructionSuggestionUpgrade) + rec.Header().Set(headerCLIInstruction, CLIInstructionSuggestionVersionUpgrade) rec.WriteHeader(http.StatusOK) return rec.Result(), nil }) @@ -119,7 +119,7 @@ func TestVersionProtocolDoer_LatestInvocationWins(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/projects", http.NoBody) _, err := versionProtocolDoer{next: first}.Do(req) require.NoError(t, err) - require.Equal(t, CLIInstructionSuggestionUpgrade, LastInstructions().CLIInstruction) + require.Equal(t, CLIInstructionSuggestionVersionUpgrade, LastInstructions().CLIInstruction) _, err = versionProtocolDoer{next: second}.Do(req) require.NoError(t, err) diff --git a/internal/cmd/upgrade/upgrade.go b/internal/cmd/upgrade/upgrade.go index b3acd40..27436b4 100644 --- a/internal/cmd/upgrade/upgrade.go +++ b/internal/cmd/upgrade/upgrade.go @@ -51,9 +51,9 @@ func updateOptions(deps cliruntime.Deps) update.Options { func PrintAPIInstructionNotices(cmd *cobra.Command, deps cliruntime.Deps) { instructions := api.LastInstructions() switch instructions.CLIInstruction { - case api.CLIInstructionVersionDeprecation: + case api.CLIInstructionRequireVersionUpgrade: printDeprecationWarning(cmd, deps, instructions.LatestVersion) - case api.CLIInstructionSuggestionUpgrade: + case api.CLIInstructionSuggestionVersionUpgrade: printUpgradeSuggestion(cmd, deps, instructions.LatestVersion) } } diff --git a/internal/cmd/upgrade/upgrade_test.go b/internal/cmd/upgrade/upgrade_test.go index a616f92..2fd93b2 100644 --- a/internal/cmd/upgrade/upgrade_test.go +++ b/internal/cmd/upgrade/upgrade_test.go @@ -132,7 +132,7 @@ func TestPrintAPIInstructionNotices_Suggestion(t *testing.T) { oldVersion := version.Version version.Version = "v1.2.3" t.Cleanup(func() { version.Version = oldVersion }) - withInstructions(t, api.CLIInstructionSuggestionUpgrade, "v1.5.0", "") + withInstructions(t, api.CLIInstructionSuggestionVersionUpgrade, "v1.5.0", "") cmd := &cobra.Command{Use: "projects"} var out bytes.Buffer @@ -146,7 +146,7 @@ func TestPrintAPIInstructionNotices_SuggestionWithoutLatestVersion(t *testing.T) oldVersion := version.Version version.Version = "v1.2.3" t.Cleanup(func() { version.Version = oldVersion }) - withInstructions(t, api.CLIInstructionSuggestionUpgrade, "", "") + withInstructions(t, api.CLIInstructionSuggestionVersionUpgrade, "", "") cmd := &cobra.Command{Use: "projects"} var out bytes.Buffer @@ -160,7 +160,7 @@ func TestPrintAPIInstructionNotices_Deprecation(t *testing.T) { oldVersion := version.Version version.Version = "v0.9.0" t.Cleanup(func() { version.Version = oldVersion }) - withInstructions(t, api.CLIInstructionVersionDeprecation, "v1.5.0", "") + withInstructions(t, api.CLIInstructionRequireVersionUpgrade, "v1.5.0", "") cmd := &cobra.Command{Use: "login"} var out bytes.Buffer @@ -171,7 +171,7 @@ func TestPrintAPIInstructionNotices_Deprecation(t *testing.T) { } func TestPrintAPIInstructionNotices_UsesCommandPathPrefix(t *testing.T) { - withInstructions(t, api.CLIInstructionSuggestionUpgrade, "v1.5.0", "") + withInstructions(t, api.CLIInstructionSuggestionVersionUpgrade, "v1.5.0", "") cmd := &cobra.Command{Use: "projects"} var out bytes.Buffer From 3c0f5e3e06b63ccca1684f01a98a2ac1836d1ddb Mon Sep 17 00:00:00 2001 From: Ted Kim Date: Wed, 8 Jul 2026 19:09:56 -0400 Subject: [PATCH 3/6] feat: reintroduce reserved credit-instruction constants (VOL-180) Mirrors volcano-hosting's CLIInstructionLowCreditWarning / CLIInstructionNotEnoughCredit: reserved, unimplemented instruction values locked in now so a future billing-service integration doesn't need a wire-format rename on either side. No case for these in PrintAPIInstructionNotices yet \u2014 the API never emits them, and CLI-side handling needs its own design pass once billing integration is scoped. --- internal/api/version_protocol.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/api/version_protocol.go b/internal/api/version_protocol.go index a26c43a..6839557 100644 --- a/internal/api/version_protocol.go +++ b/internal/api/version_protocol.go @@ -29,6 +29,16 @@ const ( // CLIInstructionRequireVersionUpgrade signals this CLI version is no longer // supported. The API pairs it with an HTTP 426 on non-exempt routes. CLIInstructionRequireVersionUpgrade = "require_version_upgrade" + + // CLIInstructionLowCreditWarning and CLIInstructionNotEnoughCredit are + // RESERVED for a future billing/credit gate. The API does not emit them + // yet — billing integration into this protocol needs its own design pass — + // so there is no case for these in PrintAPIInstructionNotices either. + // Naming is locked in now, parallel to the version instructions, so a + // future implementation on both sides doesn't need a wire-format rename. + CLIInstructionLowCreditWarning = "low_credit_warning" + CLIInstructionNotEnoughCredit = "not_enough_credit" + // DeviceInstructionReauth signals the platform token needs re-authentication. DeviceInstructionReauth = "reauth" ) From 015a33b5cd868dbe67c4ec4b2154e4cd8ebb2417 Mon Sep 17 00:00:00 2001 From: Ted Kim Date: Wed, 8 Jul 2026 20:41:40 -0400 Subject: [PATCH 4/6] feat: wire a no-op credit-instruction hook for future billing integration (VOL-180) Adds real, testable handling for the reserved not_enough_credit / low_credit_warning instructions in PrintAPIInstructionNotices \u2014 not just the constants. Unreached today since the API never emits these (see volcano-hosting's matching creditgate scaffold), but once it does, this side needs zero changes to start working. The printed notice is a placeholder, not the designed UX: marckong's actual ask is an interactive prompt ("upgrade?" / "purchase extra credits?") that takes a real action, which needs a target (URL? command?) that doesn't exist yet. internal/confirm already has the prompt primitive for that once there's something concrete to confirm \u2014 noted inline so the next change is obvious. --- internal/cmd/upgrade/upgrade.go | 24 ++++++++++++++++++++++++ internal/cmd/upgrade/upgrade_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/internal/cmd/upgrade/upgrade.go b/internal/cmd/upgrade/upgrade.go index 27436b4..30f067e 100644 --- a/internal/cmd/upgrade/upgrade.go +++ b/internal/cmd/upgrade/upgrade.go @@ -55,6 +55,10 @@ func PrintAPIInstructionNotices(cmd *cobra.Command, deps cliruntime.Deps) { printDeprecationWarning(cmd, deps, instructions.LatestVersion) case api.CLIInstructionSuggestionVersionUpgrade: printUpgradeSuggestion(cmd, deps, instructions.LatestVersion) + case api.CLIInstructionNotEnoughCredit: + printNotEnoughCreditWarning(cmd) + case api.CLIInstructionLowCreditWarning: + printLowCreditWarning(cmd) } } @@ -81,3 +85,23 @@ func printDeprecationWarning(cmd *cobra.Command, deps cliruntime.Deps, latest st } fmt.Fprintf(cmd.ErrOrStderr(), "Volcano CLI %s is no longer supported. Run `%s` to upgrade.\n", version.Version, upgradeCmd) } + +// printNotEnoughCreditWarning and printLowCreditWarning handle the reserved +// credit instructions (VOL-180 PR review discussion, api.CLIInstructionNot +// EnoughCredit / api.CLIInstructionLowCreditWarning). The API never emits +// these yet — billing integration needs its own design pass — so these paths +// are unreached today; they exist so that once the server starts sending the +// header, this is already wired and nothing else needs to change here. +// +// The printed notice is a placeholder, not the designed UX: the actual ask is +// an interactive prompt ("upgrade?" / "purchase extra credits?") that takes +// an action, which needs a real target (a URL? a command?) that doesn't +// exist yet. internal/confirm already has the prompt primitive for that once +// there's something concrete to confirm. +func printNotEnoughCreditWarning(cmd *cobra.Command) { + fmt.Fprintln(cmd.ErrOrStderr(), "Your project does not have enough credit to complete this request.") +} + +func printLowCreditWarning(cmd *cobra.Command) { + fmt.Fprintln(cmd.ErrOrStderr(), "Your project is running low on credit.") +} diff --git a/internal/cmd/upgrade/upgrade_test.go b/internal/cmd/upgrade/upgrade_test.go index 2fd93b2..add1b9b 100644 --- a/internal/cmd/upgrade/upgrade_test.go +++ b/internal/cmd/upgrade/upgrade_test.go @@ -170,6 +170,31 @@ func TestPrintAPIInstructionNotices_Deprecation(t *testing.T) { assert.Contains(t, out.String(), "Volcano CLI v0.9.0 is no longer supported. Upgrade to v1.5.0 or later:\n volcano upgrade") } +func TestPrintAPIInstructionNotices_NotEnoughCredit(t *testing.T) { + // Reserved instruction (VOL-180 PR review discussion): the API never + // emits this yet, but the CLI-side handling is real and testable so a + // future billing-service integration needs no CLI change to start working. + withInstructions(t, api.CLIInstructionNotEnoughCredit, "", "") + + cmd := &cobra.Command{Use: "functions"} + var out bytes.Buffer + cmd.SetErr(&out) + PrintAPIInstructionNotices(cmd, cliruntime.Deps{}) + + assert.Contains(t, out.String(), "Your project does not have enough credit to complete this request.") +} + +func TestPrintAPIInstructionNotices_LowCreditWarning(t *testing.T) { + withInstructions(t, api.CLIInstructionLowCreditWarning, "", "") + + cmd := &cobra.Command{Use: "functions"} + var out bytes.Buffer + cmd.SetErr(&out) + PrintAPIInstructionNotices(cmd, cliruntime.Deps{}) + + assert.Contains(t, out.String(), "Your project is running low on credit.") +} + func TestPrintAPIInstructionNotices_UsesCommandPathPrefix(t *testing.T) { withInstructions(t, api.CLIInstructionSuggestionVersionUpgrade, "v1.5.0", "") From 7deb47c5a3e72184c7b06a353e266ab3059f3148 Mon Sep 17 00:00:00 2001 From: Ted Kim Date: Wed, 8 Jul 2026 21:42:16 -0400 Subject: [PATCH 5/6] fix: address code review findings on PR #39 (VOL-180) All 6 findings verified against the actual code before fixing. 1. [P3] Fixed a stale comment claiming PrintAPIInstructionNotices has no case for the reserved credit instructions \u2014 the credit-hook commit added exactly those cases, but the comment written before that landed was never updated. 2. [P3] recordInstructions previously overwrote the full Instructions snapshot on every response, so a command making multiple API calls could have a real notice (suggestion, deprecation, or reauth) silently dropped by a later, unrelated response that simply didn't repeat the header. Now merges per-field: CLIInstruction and its paired LatestVersion update together (they come from the same server-side gate decision), DeviceInstruction updates independently, and neither is cleared by an empty value \u2014 only ever replaced by a new non-empty one. Added ResetLastInstructionsForTest (exported, test-only) since the sticky behavior means tests can no longer rely on an unrelated response to reset state between cases. 3. [nit] main's non-blocking-error path printed the pending notice before the actual "Error:" line. Reordered so the failure always prints first \u2014 both for humans skimming stderr and for log parsers/scripts that assume line 1 is the error. 4. [nit] Extracted main()'s orchestration (426 short-circuit, error-before- notice ordering, exit codes) into a testable run(root, deps) int function, and added tests driving it through real commands against a test server (success/no-notice, success/suggestion, 426 short-circuit without duplicating the notice, non-blocking error ordering, reauth ordering) \u2014 previously only the extracted print helpers were tested, not this orchestration itself. 5. [nit] Fixed a comment that wrapped api.CLIInstructionNotEnoughCredit across two lines mid-identifier, reading like a typo. 6. [nit] Removed the now-unused update.Notice type, the last remnant of the removed GitHub-polling notice model. Verified: go build/vet clean, full test suite green (including -count=5 -shuffle=on on the changed package to rule out the new sticky-state cross-test contamination this surfaced and fixed), make lint 0 issues, make tidy no go.mod/go.sum diff. --- cmd/volcano/main.go | 44 +++++-- cmd/volcano/main_test.go | 161 ++++++++++++++++++++++++++ internal/api/version_protocol.go | 55 +++++++-- internal/api/version_protocol_test.go | 82 +++++++++++-- internal/cmd/upgrade/upgrade.go | 5 +- internal/cmd/upgrade/upgrade_test.go | 4 + internal/update/update.go | 6 - 7 files changed, 313 insertions(+), 44 deletions(-) diff --git a/cmd/volcano/main.go b/cmd/volcano/main.go index ece608f..6d1dcbf 100644 --- a/cmd/volcano/main.go +++ b/cmd/volcano/main.go @@ -7,6 +7,8 @@ import ( "net/http" "os" + "github.com/spf13/cobra" + "github.com/Kong/volcano-cli/internal/api" rootcmd "github.com/Kong/volcano-cli/internal/cmd/root" upgradecmd "github.com/Kong/volcano-cli/internal/cmd/upgrade" @@ -15,29 +17,47 @@ import ( func main() { deps := cliruntime.Deps{} - root := rootcmd.New(deps) + os.Exit(run(rootcmd.New(deps), deps)) +} + +// run executes root and returns the process exit code. Extracted from main +// so this orchestration — 426 short-circuiting, error-before-notices +// ordering, and exit codes — is covered by tests instead of only by the +// individually-tested helper functions it calls. Uses root.ErrOrStderr() +// (defaults to os.Stderr, same as before this extraction) so tests can +// redirect it via root.SetErr. +func run(root *cobra.Command, deps cliruntime.Deps) int { err := root.Execute() + stderr := root.ErrOrStderr() if err != nil && api.Status(err) == http.StatusUpgradeRequired { // The 426 body's message already reads "cli version no longer // supported; run `volcano upgrade`"; just add the concrete upgrade // target when the API provided one, and stop — printing the // suggestion/deprecation notice below too would repeat ourselves. - printDeprecationError(os.Stderr, err, deps) - os.Exit(1) + printDeprecationError(stderr, err, deps) + return 1 } - // Print any VOL-180 upgrade/deprecation notice unconditionally otherwise: - // covers the non-blocking suggestion, and a deprecated CLI succeeding on - // an exempt route (e.g. `login`) where the user still needs to know their - // CLI is deprecated even though this particular command was let through. - // Reads only in-process state (api.LastInstructions); adds no network call. - upgradecmd.PrintAPIInstructionNotices(root, deps) - if err != nil { - printError(os.Stderr, err, deps) - os.Exit(1) + // Print the failure first: stderr's first line should be the actual + // error, for both humans skimming and log parsers/scripts that treat + // line 1 as the failure reason. Any pending notice is secondary + // context, printed after — VOL-180 instructions observed from an + // earlier API call in this invocation are not cleared by whatever + // unrelated error ended the command (see api's recordInstructions). + printError(stderr, err, deps) + upgradecmd.PrintAPIInstructionNotices(root, deps) + return 1 } + + // Success: covers the non-blocking suggestion, and a deprecated CLI + // succeeding on an exempt route (e.g. `login`) where the user still needs + // to know their CLI is deprecated even though this command was let + // through. Reads only in-process state (api.LastInstructions); adds no + // network call. + upgradecmd.PrintAPIInstructionNotices(root, deps) + return 0 } // printDeprecationError renders a require_version_upgrade 426 error together with diff --git a/cmd/volcano/main_test.go b/cmd/volcano/main_test.go index 89d3d02..aa82041 100644 --- a/cmd/volcano/main_test.go +++ b/cmd/volcano/main_test.go @@ -5,12 +5,15 @@ import ( "context" "net/http" "net/http/httptest" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/Kong/volcano-cli/internal/api" + rootcmd "github.com/Kong/volcano-cli/internal/cmd/root" + cliconfig "github.com/Kong/volcano-cli/internal/config" cliruntime "github.com/Kong/volcano-cli/internal/runtime" ) @@ -19,8 +22,17 @@ import ( // response headers (VOL-180). func withInstructions(t *testing.T, latest, deviceInstruction string) { t.Helper() + // recordInstructions is sticky (VOL-180): a field a response omits doesn't + // clear a value recorded by an earlier test. Reset explicitly so each test + // starts from the zero value regardless of execution order. + api.ResetLastInstructionsForTest() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { if latest != "" { + // The server never sends X-Volcano-CLI-Latest-Version without a + // paired instruction (setLatestVersionHeader is only called from the + // suggest/deprecate branches) — recordInstructions relies on that + // contract, so this helper must too. + w.Header().Set("X-Volcano-CLI-Instruction", api.CLIInstructionRequireVersionUpgrade) w.Header().Set("X-Volcano-CLI-Latest-Version", latest) } if deviceInstruction != "" { @@ -95,3 +107,152 @@ func TestPrintError_ReauthHintUsesCommandPathPrefix(t *testing.T) { assert.Contains(t, out.String(), "Run `acme login` to re-authenticate.") } + +// runDeps builds cliruntime.Deps + a rootcmd.New(deps) wired to server via an +// in-memory config (no disk/env), so tests can drive run() through a real +// command instead of poking internal state. +func runDeps(server *httptest.Server) cliruntime.Deps { + return cliruntime.Deps{ + HTTPClient: server.Client(), + ConfigLoader: func() (*cliconfig.Config, error) { + return &cliconfig.Config{ + APIBaseURL: server.URL, + UserToken: "test-token", + IgnoreEnv: true, + }, nil + }, + } +} + +func TestRun_SuccessNoNotice(t *testing.T) { + api.ResetLastInstructionsForTest() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":[],"has_more":false,"page":1,"limit":100,"total":0}`)) + })) + defer server.Close() + + deps := runDeps(server) + root := rootcmd.New(deps) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs([]string{"projects", "list"}) + + code := run(root, deps) + + assert.Equal(t, 0, code) + assert.Empty(t, stderr.String(), "no notice pending, stderr must stay empty") +} + +func TestRun_SuccessWithSuggestionNotice(t *testing.T) { + api.ResetLastInstructionsForTest() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-Volcano-CLI-Instruction", api.CLIInstructionSuggestionVersionUpgrade) + w.Header().Set("X-Volcano-CLI-Latest-Version", "v1.5.0") + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":[],"has_more":false,"page":1,"limit":100,"total":0}`)) + })) + defer server.Close() + + deps := runDeps(server) + root := rootcmd.New(deps) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs([]string{"projects", "list"}) + + code := run(root, deps) + + assert.Equal(t, 0, code) + assert.Contains(t, stderr.String(), "A newer Volcano CLI version is available: v1.5.0") +} + +func TestRun_DeprecationErrorShortCircuitsWithoutDuplicateNotice(t *testing.T) { + api.ResetLastInstructionsForTest() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-Volcano-CLI-Instruction", api.CLIInstructionRequireVersionUpgrade) + w.Header().Set("X-Volcano-CLI-Latest-Version", "v1.5.0") + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUpgradeRequired) + _, _ = w.Write([]byte(`{"error":"cli version no longer supported; run ` + "`volcano upgrade`" + `"}`)) + })) + defer server.Close() + + deps := runDeps(server) + root := rootcmd.New(deps) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs([]string{"projects", "list"}) + + code := run(root, deps) + + assert.Equal(t, 1, code) + text := stderr.String() + assert.Contains(t, text, "Error:") + assert.Contains(t, text, "Upgrade to v1.5.0: volcano upgrade") + // The 426 path must not ALSO print the generic non-blocking suggestion + // notice (a distinct phrase from the deprecation error's own message) — + // that would tell the user the same thing twice in different words. + assert.NotContains(t, text, "A newer Volcano CLI version is available", "deprecation error path must not duplicate the suggestion notice: %q", text) +} + +func TestRun_NonBlockingErrorPrintsErrorBeforeNotice(t *testing.T) { + // A command that fails for an unrelated reason (404 here) while also + // carrying a pending suggestion notice must print the actual error first. + api.ResetLastInstructionsForTest() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-Volcano-CLI-Instruction", api.CLIInstructionSuggestionVersionUpgrade) + w.Header().Set("X-Volcano-CLI-Latest-Version", "v1.5.0") + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"project not found"}`)) + })) + defer server.Close() + + deps := runDeps(server) + root := rootcmd.New(deps) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs([]string{"projects", "get", "11111111-1111-1111-1111-111111111111"}) + + code := run(root, deps) + + assert.Equal(t, 1, code) + text := stderr.String() + require.Contains(t, text, "Error:") + require.Contains(t, text, "newer Volcano CLI version is available") + assert.Less(t, strings.Index(text, "Error:"), strings.Index(text, "newer Volcano CLI version is available"), + "the error line must come before the notice: %q", text) +} + +func TestRun_NonBlockingErrorWithReauthHint(t *testing.T) { + api.ResetLastInstructionsForTest() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-Volcano-Device-Instruction", api.DeviceInstructionReauth) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":"token expired"}`)) + })) + defer server.Close() + + deps := runDeps(server) + root := rootcmd.New(deps) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs([]string{"projects", "list"}) + + code := run(root, deps) + + assert.Equal(t, 1, code) + text := stderr.String() + require.Contains(t, text, "Error:") + require.Contains(t, text, "Run `volcano login` to re-authenticate.") + assert.Less(t, strings.Index(text, "Error:"), strings.Index(text, "Run `volcano login`"), + "the error line must come before the reauth hint: %q", text) +} diff --git a/internal/api/version_protocol.go b/internal/api/version_protocol.go index 6839557..f9c0dfd 100644 --- a/internal/api/version_protocol.go +++ b/internal/api/version_protocol.go @@ -31,11 +31,12 @@ const ( CLIInstructionRequireVersionUpgrade = "require_version_upgrade" // CLIInstructionLowCreditWarning and CLIInstructionNotEnoughCredit are - // RESERVED for a future billing/credit gate. The API does not emit them - // yet — billing integration into this protocol needs its own design pass — - // so there is no case for these in PrintAPIInstructionNotices either. - // Naming is locked in now, parallel to the version instructions, so a - // future implementation on both sides doesn't need a wire-format rename. + // RESERVED for a future billing/credit gate. PrintAPIInstructionNotices + // already has cases for both, but they're unreached today: the API does + // not emit them yet — billing integration into this protocol needs its own + // design pass. Naming is locked in now, parallel to the version + // instructions, so a future server-side implementation doesn't need a + // wire-format rename. CLIInstructionLowCreditWarning = "low_credit_warning" CLIInstructionNotEnoughCredit = "not_enough_credit" @@ -70,15 +71,32 @@ func LastInstructions() Instructions { return lastInstructions } +// recordInstructions updates the two instruction fields independently, and +// only when a response actually carries a value for that field — a response +// with no X-Volcano-CLI-Instruction header does not clear a CLIInstruction +// (and its paired LatestVersion) recorded from an earlier response in the +// same command invocation, and likewise for DeviceInstruction. A command +// that makes several API calls must not have an earlier real notice silently +// dropped just because a later, unrelated response didn't repeat the header. +// +// CLIInstruction and LatestVersion update together (never independently): +// they come from the same server-side gate decision, so a response that sets +// one is authoritative for both, even if that response's LatestVersion is +// itself empty (e.g. no latest configured). func recordInstructions(header http.Header) { - next := Instructions{ - CLIInstruction: strings.TrimSpace(header.Get(headerCLIInstruction)), - LatestVersion: strings.TrimSpace(header.Get(headerCLILatestVersion)), - DeviceInstruction: strings.TrimSpace(header.Get(headerDeviceInstruction)), - } + cliInstruction := strings.TrimSpace(header.Get(headerCLIInstruction)) + latestVersion := strings.TrimSpace(header.Get(headerCLILatestVersion)) + deviceInstruction := strings.TrimSpace(header.Get(headerDeviceInstruction)) + instructionsMu.Lock() - lastInstructions = next - instructionsMu.Unlock() + defer instructionsMu.Unlock() + if cliInstruction != "" { + lastInstructions.CLIInstruction = cliInstruction + lastInstructions.LatestVersion = latestVersion + } + if deviceInstruction != "" { + lastInstructions.DeviceInstruction = deviceInstruction + } } // userAgent identifies this CLI build to the API — for support/debugging and @@ -107,3 +125,16 @@ func (d versionProtocolDoer) Do(req *http.Request) (*http.Response, error) { } return resp, err } + +// ResetLastInstructionsForTest clears the process-global instructions state. +// Test-only: production never needs this, since each CLI invocation is a +// fresh process. Call it between test cases that expect LastInstructions() to +// start from the zero value — recordInstructions is intentionally sticky (see +// its doc comment): a response that omits a field never clears a previously +// recorded value for it, so tests must reset explicitly rather than relying +// on an unrelated response to clear prior state. +func ResetLastInstructionsForTest() { + instructionsMu.Lock() + lastInstructions = Instructions{} + instructionsMu.Unlock() +} diff --git a/internal/api/version_protocol_test.go b/internal/api/version_protocol_test.go index ab2277b..5c2d50f 100644 --- a/internal/api/version_protocol_test.go +++ b/internal/api/version_protocol_test.go @@ -16,14 +16,8 @@ import ( // cases (production never needs this: each CLI invocation is a fresh process). func resetInstructions(t *testing.T) { t.Helper() - instructionsMu.Lock() - lastInstructions = Instructions{} - instructionsMu.Unlock() - t.Cleanup(func() { - instructionsMu.Lock() - lastInstructions = Instructions{} - instructionsMu.Unlock() - }) + ResetLastInstructionsForTest() + t.Cleanup(ResetLastInstructionsForTest) } func TestVersionProtocolDoer_SetsVersionHeaderAndUserAgent(t *testing.T) { @@ -99,14 +93,16 @@ func TestVersionProtocolDoer_NoResponseIsNoOp(t *testing.T) { assert.Equal(t, Instructions{}, LastInstructions()) } -func TestVersionProtocolDoer_LatestInvocationWins(t *testing.T) { - // A CLI process runs one command per invocation; "most recent" reflects the - // last response observed within that single run. +func TestVersionProtocolDoer_LaterEmptyResponseDoesNotClearEarlierInstruction(t *testing.T) { + // A command can make several API calls; a later response that simply + // doesn't repeat the instruction header (e.g. an unrelated call) must not + // silently drop a real notice observed earlier in the same invocation. resetInstructions(t) first := doerFunc(func(*http.Request) (*http.Response, error) { rec := httptest.NewRecorder() rec.Header().Set(headerCLIInstruction, CLIInstructionSuggestionVersionUpgrade) + rec.Header().Set(headerCLILatestVersion, "v1.5.0") rec.WriteHeader(http.StatusOK) return rec.Result(), nil }) @@ -120,10 +116,72 @@ func TestVersionProtocolDoer_LatestInvocationWins(t *testing.T) { _, err := versionProtocolDoer{next: first}.Do(req) require.NoError(t, err) require.Equal(t, CLIInstructionSuggestionVersionUpgrade, LastInstructions().CLIInstruction) + require.Equal(t, "v1.5.0", LastInstructions().LatestVersion) + + _, err = versionProtocolDoer{next: second}.Do(req) + require.NoError(t, err) + assert.Equal(t, CLIInstructionSuggestionVersionUpgrade, LastInstructions().CLIInstruction, "a later response with no instruction header must not clear an earlier real one") + assert.Equal(t, "v1.5.0", LastInstructions().LatestVersion, "LatestVersion stays paired with the preserved CLIInstruction") +} + +func TestVersionProtocolDoer_LaterNonEmptyResponseUpdatesInstruction(t *testing.T) { + // A genuinely different instruction (e.g. the policy changed, or a later + // call reports a different version comparison) must still take effect — + // preservation only protects against being cleared by an empty response. + resetInstructions(t) + + first := doerFunc(func(*http.Request) (*http.Response, error) { + rec := httptest.NewRecorder() + rec.Header().Set(headerCLIInstruction, CLIInstructionSuggestionVersionUpgrade) + rec.Header().Set(headerCLILatestVersion, "v1.5.0") + rec.WriteHeader(http.StatusOK) + return rec.Result(), nil + }) + second := doerFunc(func(*http.Request) (*http.Response, error) { + rec := httptest.NewRecorder() + rec.Header().Set(headerCLIInstruction, CLIInstructionRequireVersionUpgrade) + rec.Header().Set(headerCLILatestVersion, "v1.6.0") + rec.WriteHeader(http.StatusUpgradeRequired) + return rec.Result(), nil + }) + + req := httptest.NewRequest(http.MethodGet, "/projects", http.NoBody) + _, err := versionProtocolDoer{next: first}.Do(req) + require.NoError(t, err) + _, err = versionProtocolDoer{next: second}.Do(req) + require.NoError(t, err) + + assert.Equal(t, CLIInstructionRequireVersionUpgrade, LastInstructions().CLIInstruction) + assert.Equal(t, "v1.6.0", LastInstructions().LatestVersion) +} + +func TestVersionProtocolDoer_CLIInstructionAndDeviceInstructionTrackIndependently(t *testing.T) { + // A response that sets DeviceInstruction but not CLIInstruction (or vice + // versa) must not clear whichever one it didn't mention. + resetInstructions(t) + + first := doerFunc(func(*http.Request) (*http.Response, error) { + rec := httptest.NewRecorder() + rec.Header().Set(headerCLIInstruction, CLIInstructionSuggestionVersionUpgrade) + rec.WriteHeader(http.StatusOK) + return rec.Result(), nil + }) + second := doerFunc(func(*http.Request) (*http.Response, error) { + rec := httptest.NewRecorder() + rec.Header().Set(headerDeviceInstruction, DeviceInstructionReauth) + rec.WriteHeader(http.StatusUnauthorized) + return rec.Result(), nil + }) + req := httptest.NewRequest(http.MethodGet, "/projects", http.NoBody) + _, err := versionProtocolDoer{next: first}.Do(req) + require.NoError(t, err) _, err = versionProtocolDoer{next: second}.Do(req) require.NoError(t, err) - assert.Empty(t, LastInstructions().CLIInstruction, "a later response with no instruction header clears the prior one") + + got := LastInstructions() + assert.Equal(t, CLIInstructionSuggestionVersionUpgrade, got.CLIInstruction, "unrelated device-instruction response must not clear the earlier CLI instruction") + assert.Equal(t, DeviceInstructionReauth, got.DeviceInstruction) } type doerFunc func(*http.Request) (*http.Response, error) diff --git a/internal/cmd/upgrade/upgrade.go b/internal/cmd/upgrade/upgrade.go index 30f067e..2275bdf 100644 --- a/internal/cmd/upgrade/upgrade.go +++ b/internal/cmd/upgrade/upgrade.go @@ -87,8 +87,9 @@ func printDeprecationWarning(cmd *cobra.Command, deps cliruntime.Deps, latest st } // printNotEnoughCreditWarning and printLowCreditWarning handle the reserved -// credit instructions (VOL-180 PR review discussion, api.CLIInstructionNot -// EnoughCredit / api.CLIInstructionLowCreditWarning). The API never emits +// credit instructions (VOL-180 PR review discussion, +// api.CLIInstructionNotEnoughCredit / api.CLIInstructionLowCreditWarning). +// The API never emits // these yet — billing integration needs its own design pass — so these paths // are unreached today; they exist so that once the server starts sending the // header, this is already wired and nothing else needs to change here. diff --git a/internal/cmd/upgrade/upgrade_test.go b/internal/cmd/upgrade/upgrade_test.go index add1b9b..2621acc 100644 --- a/internal/cmd/upgrade/upgrade_test.go +++ b/internal/cmd/upgrade/upgrade_test.go @@ -106,6 +106,10 @@ func TestUpgradeCommandRejectsArgs(t *testing.T) { // api's unexported state. func withInstructions(t *testing.T, cliInstruction, latest, deviceInstruction string) { t.Helper() + // recordInstructions is sticky (VOL-180): a field a response omits doesn't + // clear a value recorded by an earlier test. Reset explicitly so each test + // starts from the zero value regardless of execution order. + api.ResetLastInstructionsForTest() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { if cliInstruction != "" { w.Header().Set("X-Volcano-CLI-Instruction", cliInstruction) diff --git a/internal/update/update.go b/internal/update/update.go index 99debe2..ad96832 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -56,12 +56,6 @@ type Options struct { RequireSignatureVerification bool } -// Notice describes an available upgrade. -type Notice struct { - Current string - Latest string -} - // Release is the subset of GitHub release metadata the CLI needs. type Release struct { TagName string `json:"tag_name"` From 8fd11321bb663508b7629b5e54727b1a4660c7c6 Mon Sep 17 00:00:00 2001 From: Ted Kim Date: Wed, 8 Jul 2026 22:43:37 -0400 Subject: [PATCH 6/6] test: cover deprecation notice on the success/exempt-route path (VOL-180) The prior run() tests covered the 426 (blocking) deprecation path and the suggestion notice on success, but not the exact scenario printDeprecationWarning's own doc comment calls out as the reason the notice-after-success print exists at all: a deprecated CLI succeeding on an exempt route (e.g. login), where the server sets require_version_upgrade without a 426. Only PrintAPIInstructionNotices' isolated unit test covered that instruction value before \u2014 not the full run() orchestration (exit code 0, notice still printed). Verified: full suite green, -count=5 -shuffle=on on cmd/volcano clean, make lint 0 issues, make tidy no go.mod/go.sum diff. --- cmd/volcano/main_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/cmd/volcano/main_test.go b/cmd/volcano/main_test.go index aa82041..a0f433e 100644 --- a/cmd/volcano/main_test.go +++ b/cmd/volcano/main_test.go @@ -170,6 +170,41 @@ func TestRun_SuccessWithSuggestionNotice(t *testing.T) { assert.Contains(t, stderr.String(), "A newer Volcano CLI version is available: v1.5.0") } +func TestRun_SuccessWithDeprecationNoticeOnExemptRoute(t *testing.T) { + // The exact scenario the notice-printing path exists for (per + // printDeprecationWarning's doc comment): a deprecated CLI's request lands + // on an exempt route (e.g. `login`) and succeeds — the server sets the + // require_version_upgrade header but doesn't 426 it. The user still needs + // to learn their CLI is deprecated even though this command was let + // through. Only the deprecation-error (426) path had run()-level coverage + // before this; this is the success-path counterpart. + api.ResetLastInstructionsForTest() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-Volcano-CLI-Instruction", api.CLIInstructionRequireVersionUpgrade) + w.Header().Set("X-Volcano-CLI-Latest-Version", "v1.5.0") + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":[],"has_more":false,"page":1,"limit":100,"total":0}`)) + })) + defer server.Close() + + deps := runDeps(server) + root := rootcmd.New(deps) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs([]string{"projects", "list"}) + + code := run(root, deps) + + // The command succeeded (no error to short-circuit on), so the exit code + // must be 0 even though the CLI is deprecated — only the 426 path (a + // non-exempt route) exits 1. + assert.Equal(t, 0, code) + assert.Contains(t, stderr.String(), "Volcano CLI") + assert.Contains(t, stderr.String(), "is no longer supported. Upgrade to v1.5.0 or later:") +} + func TestRun_DeprecationErrorShortCircuitsWithoutDuplicateNotice(t *testing.T) { api.ResetLastInstructionsForTest() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {