From 7fe485344b995c305308b9d83708d8188d669247 Mon Sep 17 00:00:00 2001 From: Ted Kim Date: Thu, 9 Jul 2026 17:16:11 -0400 Subject: [PATCH 1/3] fix(cli): handle version-gate followups --- cmd/volcano/main_test.go | 111 +++++++++++++++++++++----- internal/api/log_stream_test.go | 11 +++ internal/api/version_protocol.go | 87 +++++++++++++++++--- internal/api/version_protocol_test.go | 47 +++++++++++ internal/cmd/frontends/logs.go | 10 ++- internal/cmd/frontends/logs_test.go | 8 +- internal/cmd/functions/logs.go | 10 ++- internal/cmd/functions/logs_test.go | 8 +- internal/cmd/upgrade/upgrade.go | 2 +- internal/cmd/upgrade/upgrade_test.go | 15 ++++ internal/logfollow/follow.go | 19 ++++- 11 files changed, 290 insertions(+), 38 deletions(-) diff --git a/cmd/volcano/main_test.go b/cmd/volcano/main_test.go index a0f433e..0a555d6 100644 --- a/cmd/volcano/main_test.go +++ b/cmd/volcano/main_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -17,15 +18,20 @@ import ( cliruntime "github.com/Kong/volcano-cli/internal/runtime" ) +func resetInstructions(t *testing.T) { + t.Helper() + api.ResetLastInstructionsForTest() + t.Cleanup(api.ResetLastInstructionsForTest) +} + // 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() - // 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() + // recordInstructions is sticky (VOL-180), so isolate both the beginning and + // end of each test from the process-global state. + resetInstructions(t) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { if latest != "" { // The server never sends X-Volcano-CLI-Latest-Version without a @@ -125,7 +131,7 @@ func runDeps(server *httptest.Server) cliruntime.Deps { } func TestRun_SuccessNoNotice(t *testing.T) { - api.ResetLastInstructionsForTest() + resetInstructions(t) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) @@ -147,7 +153,7 @@ func TestRun_SuccessNoNotice(t *testing.T) { } func TestRun_SuccessWithSuggestionNotice(t *testing.T) { - api.ResetLastInstructionsForTest() + resetInstructions(t) 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") @@ -170,15 +176,10 @@ 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() +func TestRun_SuccessWithDeprecationNotice(t *testing.T) { + // A successful API response can still carry a deprecation instruction. The + // command must warn without changing its successful exit status. + resetInstructions(t) 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") @@ -205,8 +206,69 @@ func TestRun_SuccessWithDeprecationNoticeOnExemptRoute(t *testing.T) { assert.Contains(t, stderr.String(), "is no longer supported. Upgrade to v1.5.0 or later:") } +func TestRun_SuccessWithDeprecationNoticeOnExemptAuthRoute(t *testing.T) { + resetInstructions(t) + t.Setenv("HOME", t.TempDir()) + t.Setenv("VOLCANO_TOKEN", "") + t.Setenv("VOLCANO_API_URL", "") + t.Setenv("VOLCANO_FIRST_PARTY_DEVICE_CLIENT_ID", "") + + pollTicker := newMainTestTicker() + dotTicker := newMainTestTicker() + timeoutTimer := newMainTestTicker() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *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") + switch r.URL.Path { + case "/auth/device/authorize": + _, _ = w.Write([]byte(`{"device_code":"device-code","user_code":"ABCD-EFGH","verification_uri":"https://volcano.dev/device","verification_uri_complete":"https://volcano.dev/device?user_code=ABCD-EFGH","expires_in":120,"interval":1}`)) + case "/auth/device/token": + _, _ = w.Write([]byte(`{"access_token":"auth-access-token"}`)) + case "/auth/platform/exchange": + _, _ = w.Write([]byte(`{"token":"platform-token","user_id":"platform-user-1","token_id":"33333333-3333-4333-8333-333333333333","expires_at":"2030-01-01T00:00:00Z"}`)) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + var tickerCalls int + deps := cliruntime.Deps{ + HTTPClient: server.Client(), + APIBaseURL: server.URL, + OpenBrowser: func(string) error { return nil }, + NewTimer: func(time.Duration) cliruntime.Timer { return timeoutTimer }, + NewTicker: func(time.Duration) cliruntime.Ticker { + tickerCalls++ + if tickerCalls == 1 { + return pollTicker + } + return dotTicker + }, + } + root := rootcmd.New(deps) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs([]string{"login"}) + + done := make(chan int, 1) + go func() { done <- run(root, deps) }() + pollTicker.tick() + select { + case code := <-done: + assert.Equal(t, 0, code) + case <-time.After(2 * time.Second): + t.Fatal("login did not complete") + } + assert.Contains(t, stderr.String(), "Volcano CLI") + assert.Contains(t, stderr.String(), "is no longer supported. Upgrade to v1.5.0 or later:") + assert.Equal(t, 1, strings.Count(stderr.String(), "Volcano CLI"), "the early auth-route notice must not be repeated after Execute returns") +} + func TestRun_DeprecationErrorShortCircuitsWithoutDuplicateNotice(t *testing.T) { - api.ResetLastInstructionsForTest() + resetInstructions(t) 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") @@ -238,7 +300,7 @@ func TestRun_DeprecationErrorShortCircuitsWithoutDuplicateNotice(t *testing.T) { 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() + resetInstructions(t) 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") @@ -266,7 +328,7 @@ func TestRun_NonBlockingErrorPrintsErrorBeforeNotice(t *testing.T) { } func TestRun_NonBlockingErrorWithReauthHint(t *testing.T) { - api.ResetLastInstructionsForTest() + resetInstructions(t) 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") @@ -291,3 +353,16 @@ func TestRun_NonBlockingErrorWithReauthHint(t *testing.T) { assert.Less(t, strings.Index(text, "Error:"), strings.Index(text, "Run `volcano login`"), "the error line must come before the reauth hint: %q", text) } + +type mainTestTicker struct { + ch chan time.Time +} + +func newMainTestTicker() *mainTestTicker { + return &mainTestTicker{ch: make(chan time.Time, 1)} +} + +func (t *mainTestTicker) C() <-chan time.Time { return t.ch } +func (t *mainTestTicker) Stop() {} +func (t *mainTestTicker) Reset(time.Duration) {} +func (t *mainTestTicker) tick() { t.ch <- time.Now() } diff --git a/internal/api/log_stream_test.go b/internal/api/log_stream_test.go index 569f747..af176ec 100644 --- a/internal/api/log_stream_test.go +++ b/internal/api/log_stream_test.go @@ -11,9 +11,12 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/Kong/volcano-cli/internal/version" ) func TestStreamProjectLogsRequestAndEvents(t *testing.T) { + resetInstructions(t) projectID := uuid.MustParse("11111111-1111-4111-8111-111111111111") functionID := uuid.MustParse("22222222-2222-4222-8222-222222222222") deploymentID := uuid.MustParse("33333333-3333-4333-8333-333333333333") @@ -25,7 +28,11 @@ func TestStreamProjectLogsRequestAndEvents(t *testing.T) { assert.Equal(t, "/volcano-api/projects/"+projectID.String()+"/logs/stream", r.URL.Path) assert.Equal(t, "Bearer token", r.Header.Get("Authorization")) assert.Equal(t, "text/event-stream", r.Header.Get("Accept")) + assert.Equal(t, version.Version, r.Header.Get(headerCLIVersion)) + assert.Contains(t, r.Header.Get("User-Agent"), "volcano-cli/"+version.Version) lastEventID = r.Header.Get("Last-Event-ID") + w.Header().Set(headerCLIInstruction, CLIInstructionSuggestionVersionUpgrade) + w.Header().Set(headerCLILatestVersion, "v1.5.0") require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) w.Header().Set("Content-Type", "text/event-stream") @@ -46,6 +53,10 @@ func TestStreamProjectLogsRequestAndEvents(t *testing.T) { defer stream.Close() assert.Equal(t, "previous-id", lastEventID) + assert.Equal(t, Instructions{ + CLIInstruction: CLIInstructionSuggestionVersionUpgrade, + LatestVersion: "v1.5.0", + }, LastInstructions()) resource, ok := body["resource"].(map[string]any) require.True(t, ok) assert.Equal(t, "function", resource["type"]) diff --git a/internal/api/version_protocol.go b/internal/api/version_protocol.go index f9c0dfd..b2cc3ac 100644 --- a/internal/api/version_protocol.go +++ b/internal/api/version_protocol.go @@ -10,14 +10,11 @@ import ( "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). +// Header names and instruction values for the CLI version protocol. This is +// the CLI's HTTP wire contract and is duplicated here because this repository +// has no shared Go module with the API. The claimed version is advisory input +// only and never a privilege: the CLI cannot make this request header +// trustworthy, and it does not use the value to grant authority. const ( headerCLIVersion = "X-Volcano-CLI-Version" headerCLIInstruction = "X-Volcano-CLI-Instruction" @@ -58,9 +55,11 @@ var ( ) // 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. +// from any API response in this process. CLIInstruction and LatestVersion are +// cleared after ConsumeCLIInstructions renders their one-shot notice. It is +// otherwise 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 @@ -71,6 +70,19 @@ func LastInstructions() Instructions { return lastInstructions } +// ConsumeCLIInstructions returns the observed instructions and clears the +// one-shot CLI instruction and its paired latest version. It deliberately +// preserves DeviceInstruction because callers render the reauthentication hint +// alongside the command error before they render CLI notices. +func ConsumeCLIInstructions() Instructions { + instructionsMu.Lock() + defer instructionsMu.Unlock() + instructions := lastInstructions + lastInstructions.CLIInstruction = "" + lastInstructions.LatestVersion = "" + return instructions +} + // 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 @@ -106,6 +118,57 @@ func userAgent() string { return "volcano-cli/" + version.Version + " (" + runtime.GOOS + "/" + runtime.GOARCH + ")" } +// reportedCLIVersion avoids accidentally presenting a source build as a +// prerelease of its nearest tag. `git describe --tags --always --dirty` emits +// values like v1.4.2-6-gabcdef0 (or a -dirty suffix), which semver correctly +// orders below v1.4.2 even though the source is newer. Report source-build +// identifiers as dev while retaining the full build identifier in User-Agent +// for support diagnostics. +func reportedCLIVersion() string { + value := strings.TrimSpace(version.Version) + if isGitDescribeBuild(value) || isGitSHA(value) { + return "dev" + } + return value +} + +func isGitDescribeBuild(value string) bool { + if strings.HasSuffix(value, "-dirty") { + return true + } + marker := strings.LastIndex(value, "-g") + if marker < 1 { + return false + } + sha := value[marker+2:] + if len(sha) < 7 || !isHex(sha) { + return false + } + distanceStart := strings.LastIndex(value[:marker], "-") + if distanceStart < 1 { + return false + } + for _, r := range value[distanceStart+1 : marker] { + if r < '0' || r > '9' { + return false + } + } + return true +} + +func isGitSHA(value string) bool { + return len(value) >= 7 && isHex(value) +} + +func isHex(value string) bool { + for _, r := range value { + if (r < '0' || r > '9') && (r < 'a' || r > 'f') && (r < 'A' || r > 'F') { + return false + } + } + return true +} + // 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 @@ -117,7 +180,7 @@ type versionProtocolDoer struct { } func (d versionProtocolDoer) Do(req *http.Request) (*http.Response, error) { - req.Header.Set(headerCLIVersion, version.Version) + req.Header.Set(headerCLIVersion, reportedCLIVersion()) req.Header.Set("User-Agent", userAgent()) resp, err := d.next.Do(req) if resp != nil { diff --git a/internal/api/version_protocol_test.go b/internal/api/version_protocol_test.go index 5c2d50f..08721cf 100644 --- a/internal/api/version_protocol_test.go +++ b/internal/api/version_protocol_test.go @@ -41,6 +41,37 @@ func TestVersionProtocolDoer_SetsVersionHeaderAndUserAgent(t *testing.T) { assert.Equal(t, "volcano-cli/v1.4.2 ("+runtime.GOOS+"/"+runtime.GOARCH+")", gotUA) } +func TestVersionProtocolDoer_ReportsGitDescribeBuildsAsDev(t *testing.T) { + cases := []struct { + name string + version string + want string + }{ + {name: "tagged release", version: "v1.4.2", want: "v1.4.2"}, + {name: "commit after tag", version: "v1.4.2-6-gabcdef0", want: "dev"}, + {name: "dirty tag", version: "v1.4.2-dirty", want: "dev"}, + {name: "detached source revision", version: "abcdef0", want: "dev"}, + {name: "published prerelease", version: "v1.5.0-rc.1", want: "v1.5.0-rc.1"}, + {name: "nightly release", version: "v0.0.42-nightly.20260710.1", want: "v0.0.42-nightly.20260710.1"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resetInstructions(t) + oldVersion := version.Version + version.Version = tc.version + t.Cleanup(func() { version.Version = oldVersion }) + + inner := doerFunc(func(req *http.Request) (*http.Response, error) { + assert.Equal(t, tc.want, req.Header.Get(headerCLIVersion)) + return httptest.NewRecorder().Result(), nil + }) + _, err := versionProtocolDoer{next: inner}.Do(httptest.NewRequest(http.MethodGet, "/projects", http.NoBody)) + require.NoError(t, err) + }) + } +} + func TestVersionProtocolDoer_RecordsInstructionHeaders(t *testing.T) { resetInstructions(t) @@ -93,6 +124,22 @@ func TestVersionProtocolDoer_NoResponseIsNoOp(t *testing.T) { assert.Equal(t, Instructions{}, LastInstructions()) } +func TestConsumeCLIInstructionsClearsOnlyCLIFields(t *testing.T) { + resetInstructions(t) + + header := http.Header{} + header.Set(headerCLIInstruction, CLIInstructionSuggestionVersionUpgrade) + header.Set(headerCLILatestVersion, "v1.5.0") + header.Set(headerDeviceInstruction, DeviceInstructionReauth) + recordInstructions(header) + + got := ConsumeCLIInstructions() + assert.Equal(t, CLIInstructionSuggestionVersionUpgrade, got.CLIInstruction) + assert.Equal(t, "v1.5.0", got.LatestVersion) + assert.Equal(t, DeviceInstructionReauth, got.DeviceInstruction) + assert.Equal(t, Instructions{DeviceInstruction: DeviceInstructionReauth}, LastInstructions()) +} + 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 diff --git a/internal/cmd/frontends/logs.go b/internal/cmd/frontends/logs.go index 39b0b46..0eaa139 100644 --- a/internal/cmd/frontends/logs.go +++ b/internal/cmd/frontends/logs.go @@ -12,6 +12,7 @@ import ( "github.com/Kong/volcano-cli/internal/api" "github.com/Kong/volcano-cli/internal/apiclient" + upgradecmd "github.com/Kong/volcano-cli/internal/cmd/upgrade" clifrontend "github.com/Kong/volcano-cli/internal/frontend" "github.com/Kong/volcano-cli/internal/logfollow" "github.com/Kong/volcano-cli/internal/output" @@ -32,6 +33,7 @@ type frontendLogsOptions struct { limit int follow bool out io.Writer + printNotices func() } func newLogs(deps cliruntime.Deps) *cobra.Command { @@ -55,6 +57,7 @@ func newLogs(deps cliruntime.Deps) *cobra.Command { limit: limit, follow: follow, out: cmd.OutOrStdout(), + printNotices: func() { upgradecmd.PrintAPIInstructionNotices(cmd, deps) }, }) }, } @@ -85,9 +88,9 @@ func runLogs(ctx context.Context, opts frontendLogsOptions) error { if logsType == frontendLogsTypeRuntime { if opts.follow { fmt.Fprintf(opts.out, "Following runtime logs for frontend %s\n\n", frontend.Name) - return logfollow.Runtime(ctx, opts.deps, opts.out, func(ctx context.Context, lastEventID string) (*api.ProjectLogStream, error) { + return logfollow.RuntimeWithStreamOpened(ctx, opts.deps, opts.out, func(ctx context.Context, lastEventID string) (*api.ProjectLogStream, error) { return service.StreamRuntimeLogs(ctx, frontend.Id, opts.limit, lastEventID) - }) + }, opts.printNotices) } fmt.Fprintf(opts.out, "Fetching runtime logs for frontend %s\n\n", frontend.Name) return output.PrintSearchLogs(opts.out, func(cursor string) (*apiclient.LogSearchResponse, error) { @@ -138,6 +141,9 @@ func followDeploymentLogs(ctx context.Context, opts frontendLogsOptions, service cancel() return err } + if opts.printNotices != nil { + opts.printNotices() + } return logfollow.Deployment(ctx, opts.deps, opts.out, stream, cancel, func(ctx context.Context) (bool, error) { deployment, err := service.ResolveDeployment(ctx, frontendID, deploymentID.String()) if err != nil { diff --git a/internal/cmd/frontends/logs_test.go b/internal/cmd/frontends/logs_test.go index a0fa4e3..5d94133 100644 --- a/internal/cmd/frontends/logs_test.go +++ b/internal/cmd/frontends/logs_test.go @@ -12,6 +12,7 @@ import ( "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" ) @@ -64,6 +65,8 @@ func TestFrontendsLogs(t *testing.T) { }) t.Run("runtime follow streams", func(t *testing.T) { + api.ResetLastInstructionsForTest() + t.Cleanup(api.ResetLastInstructionsForTest) setFrontendCommandTestHome(t) saveFrontendCommandTestConfig(t) var streamBody map[string]any @@ -79,6 +82,8 @@ func TestFrontendsLogs(t *testing.T) { }) case r.Method == http.MethodPost && r.URL.Path == "/projects/"+frontendProjectID+"/logs/stream": require.NoError(t, json.NewDecoder(r.Body).Decode(&streamBody)) + w.Header().Set("X-Volcano-CLI-Instruction", api.CLIInstructionSuggestionVersionUpgrade) + w.Header().Set("X-Volcano-CLI-Latest-Version", "v1.5.0") writeFrontendLogStream(t, w, "runtime follow") // A healthy backend holds the connection open and tails new // events, so keep it open until the client cancels. @@ -97,7 +102,8 @@ func TestFrontendsLogs(t *testing.T) { out, errCh := streamFrontendsCommand(ctx, New(cliruntime.Deps{HTTPClient: server.Client(), APIBaseURL: server.URL}), "logs", "web", "--type", "runtime", "--follow", "--limit", "2") require.Eventually(t, func() bool { - return strings.Contains(out.String(), "runtime follow") + text := out.String() + return strings.Contains(text, "runtime follow") && strings.Contains(text, "A newer Volcano CLI version is available: v1.5.0") }, 2*time.Second, 10*time.Millisecond) cancel() require.NoError(t, <-errCh) diff --git a/internal/cmd/functions/logs.go b/internal/cmd/functions/logs.go index a26a50a..bae1eda 100644 --- a/internal/cmd/functions/logs.go +++ b/internal/cmd/functions/logs.go @@ -12,6 +12,7 @@ import ( "github.com/Kong/volcano-cli/internal/api" "github.com/Kong/volcano-cli/internal/apiclient" + upgradecmd "github.com/Kong/volcano-cli/internal/cmd/upgrade" clifunction "github.com/Kong/volcano-cli/internal/function" "github.com/Kong/volcano-cli/internal/logfollow" "github.com/Kong/volcano-cli/internal/output" @@ -32,6 +33,7 @@ type logsOptions struct { limit int follow bool out io.Writer + printNotices func() } func newLogs(deps cliruntime.Deps) *cobra.Command { @@ -55,6 +57,7 @@ func newLogs(deps cliruntime.Deps) *cobra.Command { limit: limit, follow: follow, out: cmd.OutOrStdout(), + printNotices: func() { upgradecmd.PrintAPIInstructionNotices(cmd, deps) }, }) }, } @@ -85,9 +88,9 @@ func runLogs(ctx context.Context, opts logsOptions) error { if logsType == logsTypeRuntime { if opts.follow { fmt.Fprintf(opts.out, "Following runtime logs for function %s\n\n", function.Name) - return logfollow.Runtime(ctx, opts.deps, opts.out, func(ctx context.Context, lastEventID string) (*api.ProjectLogStream, error) { + return logfollow.RuntimeWithStreamOpened(ctx, opts.deps, opts.out, func(ctx context.Context, lastEventID string) (*api.ProjectLogStream, error) { return service.StreamRuntimeLogs(ctx, function.Id, opts.limit, lastEventID) - }) + }, opts.printNotices) } fmt.Fprintf(opts.out, "Fetching runtime logs for function %s\n\n", function.Name) return output.PrintSearchLogs(opts.out, func(cursor string) (*apiclient.LogSearchResponse, error) { @@ -129,6 +132,9 @@ func followDeploymentLogs(ctx context.Context, opts logsOptions, service clifunc cancel() return err } + if opts.printNotices != nil { + opts.printNotices() + } return logfollow.Deployment(ctx, opts.deps, opts.out, stream, cancel, func(ctx context.Context) (bool, error) { deployment, err := service.ResolveDeployment(ctx, functionID, deploymentID.String()) if err != nil { diff --git a/internal/cmd/functions/logs_test.go b/internal/cmd/functions/logs_test.go index 7307b2e..79bab39 100644 --- a/internal/cmd/functions/logs_test.go +++ b/internal/cmd/functions/logs_test.go @@ -12,6 +12,7 @@ import ( "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" ) @@ -67,6 +68,8 @@ func TestFunctionsLogs(t *testing.T) { }) t.Run("runtime follow streams", func(t *testing.T) { + api.ResetLastInstructionsForTest() + t.Cleanup(api.ResetLastInstructionsForTest) setFunctionCommandTestHome(t) saveFunctionCommandTestConfig(t) var streamBody map[string]any @@ -82,6 +85,8 @@ func TestFunctionsLogs(t *testing.T) { }) case r.Method == http.MethodPost && r.URL.Path == "/projects/"+functionProjectID+"/logs/stream": require.NoError(t, json.NewDecoder(r.Body).Decode(&streamBody)) + w.Header().Set("X-Volcano-CLI-Instruction", api.CLIInstructionSuggestionVersionUpgrade) + w.Header().Set("X-Volcano-CLI-Latest-Version", "v1.5.0") writeFunctionLogStream(t, w, "runtime follow") // A healthy backend holds the connection open and tails new // events, so keep it open until the client cancels. @@ -100,7 +105,8 @@ func TestFunctionsLogs(t *testing.T) { out, errCh := streamFunctionsCommand(ctx, New(cliruntime.Deps{HTTPClient: server.Client(), APIBaseURL: server.URL}), "logs", "hello", "--type", "runtime", "--follow", "--limit", "2") require.Eventually(t, func() bool { - return strings.Contains(out.String(), "runtime follow") + text := out.String() + return strings.Contains(text, "runtime follow") && strings.Contains(text, "A newer Volcano CLI version is available: v1.5.0") }, 2*time.Second, 10*time.Millisecond) cancel() require.NoError(t, <-errCh) diff --git a/internal/cmd/upgrade/upgrade.go b/internal/cmd/upgrade/upgrade.go index 2275bdf..a3881ad 100644 --- a/internal/cmd/upgrade/upgrade.go +++ b/internal/cmd/upgrade/upgrade.go @@ -49,7 +49,7 @@ func updateOptions(deps cliruntime.Deps) update.Options { // 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() + instructions := api.ConsumeCLIInstructions() switch instructions.CLIInstruction { case api.CLIInstructionRequireVersionUpgrade: printDeprecationWarning(cmd, deps, instructions.LatestVersion) diff --git a/internal/cmd/upgrade/upgrade_test.go b/internal/cmd/upgrade/upgrade_test.go index 2621acc..c64069a 100644 --- a/internal/cmd/upgrade/upgrade_test.go +++ b/internal/cmd/upgrade/upgrade_test.go @@ -110,6 +110,7 @@ func withInstructions(t *testing.T, cliInstruction, latest, deviceInstruction st // clear a value recorded by an earlier test. Reset explicitly so each test // starts from the zero value regardless of execution order. api.ResetLastInstructionsForTest() + t.Cleanup(api.ResetLastInstructionsForTest) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { if cliInstruction != "" { w.Header().Set("X-Volcano-CLI-Instruction", cliInstruction) @@ -174,6 +175,20 @@ 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_DeprecationWithoutLatestVersion(t *testing.T) { + oldVersion := version.Version + version.Version = "v0.9.0" + t.Cleanup(func() { version.Version = oldVersion }) + withInstructions(t, api.CLIInstructionRequireVersionUpgrade, "", "") + + cmd := &cobra.Command{Use: "login"} + var out bytes.Buffer + cmd.SetErr(&out) + PrintAPIInstructionNotices(cmd, cliruntime.Deps{}) + + assert.Contains(t, out.String(), "Volcano CLI v0.9.0 is no longer supported. Run `volcano upgrade` to 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 diff --git a/internal/logfollow/follow.go b/internal/logfollow/follow.go index bf758c8..9ba6342 100644 --- a/internal/logfollow/follow.go +++ b/internal/logfollow/follow.go @@ -35,7 +35,24 @@ type StreamOpener func(ctx context.Context, lastEventID string) (*api.ProjectLog // stream cursor — resuming without replaying recent events — until the context // is canceled. A failure to open the stream is surfaced to the caller. func Runtime(ctx context.Context, deps cliruntime.Deps, w io.Writer, open StreamOpener) error { - stream := newReconnectingStream(ctx, deps, open) + return RuntimeWithStreamOpened(ctx, deps, w, open, nil) +} + +// RuntimeWithStreamOpened follows runtime logs like Runtime and invokes opened +// once after the first stream connection succeeds. Callers use this to render +// API instructions that arrive in the stream response headers before a healthy +// follow session blocks indefinitely. +func RuntimeWithStreamOpened(ctx context.Context, deps cliruntime.Deps, w io.Writer, open StreamOpener, opened func()) error { + var openedOnce sync.Once + openWithCallback := func(ctx context.Context, lastEventID string) (*api.ProjectLogStream, error) { + stream, err := open(ctx, lastEventID) + if err == nil && opened != nil { + openedOnce.Do(opened) + } + return stream, err + } + + stream := newReconnectingStream(ctx, deps, openWithCallback) defer func() { _ = stream.Close() }() From 70634f8cea3c75419213bd4a18d82cdf8009fa88 Mon Sep 17 00:00:00 2001 From: Ted Kim Date: Mon, 13 Jul 2026 14:55:03 -0400 Subject: [PATCH 2/3] fix(api): suppress repeated instruction notices --- internal/api/version_protocol.go | 49 ++++++++++++++++++++------- internal/api/version_protocol_test.go | 36 ++++++++++++++++---- 2 files changed, 66 insertions(+), 19 deletions(-) diff --git a/internal/api/version_protocol.go b/internal/api/version_protocol.go index b2cc3ac..5098f32 100644 --- a/internal/api/version_protocol.go +++ b/internal/api/version_protocol.go @@ -49,17 +49,23 @@ type Instructions struct { DeviceInstruction string } +type cliInstructionPair struct { + instruction string + latestVersion string +} + var ( - instructionsMu sync.RWMutex - lastInstructions Instructions + instructionsMu sync.RWMutex + lastInstructions Instructions + consumedCLIInstructions map[cliInstructionPair]struct{} ) // LastInstructions returns the most recently observed VOL-180 instructions -// from any API response in this process. CLIInstruction and LatestVersion are -// cleared after ConsumeCLIInstructions renders their one-shot notice. It is -// otherwise 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. +// from any API response in this process. It retains the latest observed values +// after ConsumeCLIInstructions renders their one-shot notice so error paths can +// still use response metadata such as LatestVersion. 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 @@ -70,16 +76,32 @@ func LastInstructions() Instructions { return lastInstructions } -// ConsumeCLIInstructions returns the observed instructions and clears the -// one-shot CLI instruction and its paired latest version. It deliberately -// preserves DeviceInstruction because callers render the reauthentication hint -// alongside the command error before they render CLI notices. +// ConsumeCLIInstructions returns each observed CLI instruction/latest-version +// pair once. Repeated API responses commonly carry the same pair, so retaining +// the last consumed pair prevents a long-running command from rendering the +// same notice again after a poll or stream reconnect. DeviceInstruction is +// returned unchanged because callers render the reauthentication hint from +// LastInstructions alongside command errors. func ConsumeCLIInstructions() Instructions { instructionsMu.Lock() defer instructionsMu.Unlock() + instructions := lastInstructions - lastInstructions.CLIInstruction = "" - lastInstructions.LatestVersion = "" + pair := cliInstructionPair{ + instruction: instructions.CLIInstruction, + latestVersion: instructions.LatestVersion, + } + _, consumed := consumedCLIInstructions[pair] + if pair.instruction == "" || consumed { + instructions.CLIInstruction = "" + instructions.LatestVersion = "" + return instructions + } + + if consumedCLIInstructions == nil { + consumedCLIInstructions = make(map[cliInstructionPair]struct{}) + } + consumedCLIInstructions[pair] = struct{}{} return instructions } @@ -199,5 +221,6 @@ func (d versionProtocolDoer) Do(req *http.Request) (*http.Response, error) { func ResetLastInstructionsForTest() { instructionsMu.Lock() lastInstructions = Instructions{} + consumedCLIInstructions = nil instructionsMu.Unlock() } diff --git a/internal/api/version_protocol_test.go b/internal/api/version_protocol_test.go index 08721cf..2a82846 100644 --- a/internal/api/version_protocol_test.go +++ b/internal/api/version_protocol_test.go @@ -124,7 +124,7 @@ func TestVersionProtocolDoer_NoResponseIsNoOp(t *testing.T) { assert.Equal(t, Instructions{}, LastInstructions()) } -func TestConsumeCLIInstructionsClearsOnlyCLIFields(t *testing.T) { +func TestConsumeCLIInstructionsReturnsEachPairOnce(t *testing.T) { resetInstructions(t) header := http.Header{} @@ -133,11 +133,35 @@ func TestConsumeCLIInstructionsClearsOnlyCLIFields(t *testing.T) { header.Set(headerDeviceInstruction, DeviceInstructionReauth) recordInstructions(header) - got := ConsumeCLIInstructions() - assert.Equal(t, CLIInstructionSuggestionVersionUpgrade, got.CLIInstruction) - assert.Equal(t, "v1.5.0", got.LatestVersion) - assert.Equal(t, DeviceInstructionReauth, got.DeviceInstruction) - assert.Equal(t, Instructions{DeviceInstruction: DeviceInstructionReauth}, LastInstructions()) + first := ConsumeCLIInstructions() + assert.Equal(t, Instructions{ + CLIInstruction: CLIInstructionSuggestionVersionUpgrade, + LatestVersion: "v1.5.0", + DeviceInstruction: DeviceInstructionReauth, + }, first) + assert.Equal(t, first, LastInstructions(), "consuming a notice must retain response metadata for error rendering") + + // Polls and stream reconnects repeat the version headers. Recording the + // same pair again must not make the notice pending again. + recordInstructions(header) + assert.Equal(t, Instructions{DeviceInstruction: DeviceInstructionReauth}, ConsumeCLIInstructions()) + + // A changed instruction pair is new information and is rendered once. + header.Set(headerCLIInstruction, CLIInstructionRequireVersionUpgrade) + header.Set(headerCLILatestVersion, "v1.6.0") + recordInstructions(header) + assert.Equal(t, Instructions{ + CLIInstruction: CLIInstructionRequireVersionUpgrade, + LatestVersion: "v1.6.0", + DeviceInstruction: DeviceInstructionReauth, + }, ConsumeCLIInstructions()) + assert.Equal(t, Instructions{DeviceInstruction: DeviceInstructionReauth}, ConsumeCLIInstructions()) + + // Returning to any previously rendered pair must remain suppressed. + header.Set(headerCLIInstruction, CLIInstructionSuggestionVersionUpgrade) + header.Set(headerCLILatestVersion, "v1.5.0") + recordInstructions(header) + assert.Equal(t, Instructions{DeviceInstruction: DeviceInstructionReauth}, ConsumeCLIInstructions()) } func TestVersionProtocolDoer_LaterEmptyResponseDoesNotClearEarlierInstruction(t *testing.T) { From 44e8be5d176b4a83d69f51aa8ae018902aa0c664 Mon Sep 17 00:00:00 2001 From: Ted Kim Date: Mon, 13 Jul 2026 14:55:19 -0400 Subject: [PATCH 3/3] test(api): assert normalized stream version header --- internal/api/log_stream_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/api/log_stream_test.go b/internal/api/log_stream_test.go index af176ec..2fa1db4 100644 --- a/internal/api/log_stream_test.go +++ b/internal/api/log_stream_test.go @@ -28,7 +28,7 @@ func TestStreamProjectLogsRequestAndEvents(t *testing.T) { assert.Equal(t, "/volcano-api/projects/"+projectID.String()+"/logs/stream", r.URL.Path) assert.Equal(t, "Bearer token", r.Header.Get("Authorization")) assert.Equal(t, "text/event-stream", r.Header.Get("Accept")) - assert.Equal(t, version.Version, r.Header.Get(headerCLIVersion)) + assert.Equal(t, reportedCLIVersion(), r.Header.Get(headerCLIVersion)) assert.Contains(t, r.Header.Get("User-Agent"), "volcano-cli/"+version.Version) lastEventID = r.Header.Get("Last-Event-ID") w.Header().Set(headerCLIInstruction, CLIInstructionSuggestionVersionUpgrade)