feat(api): speak the API-controlled CLI version protocol [VOL-180]#39
Open
tkkhq wants to merge 6 commits into
Open
feat(api): speak the API-controlled CLI version protocol [VOL-180]#39tkkhq wants to merge 6 commits into
tkkhq wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR moves CLI version signaling (upgrade suggestion, deprecation, reauth hint) from a GitHub-release polling mechanism to an API-driven protocol (VOL-180). The generated API client now reports the CLI version on every request and captures server instructions from response headers; the CLI then renders notices based on the observed instructions without adding extra network calls.
Changes:
- Added a VOL-180 “version protocol” transport wrapper that sets
X-Volcano-CLI-Version+ a customUser-Agent, and records instruction headers for later consumption. - Replaced GitHub-polling update notices with
PrintAPIInstructionNotices, which renders suggestion/deprecation notices based onapi.LastInstructions(). - Updated the CLI entrypoint to print notices after command execution and to enrich 426 “version_deprecation” errors with a concrete target version when provided.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/api/version_protocol.go |
Implements request header injection and response instruction recording via a transport wrapper + process-global snapshot. |
internal/api/version_protocol_test.go |
Validates header setting, instruction recording, error/no-response behavior, and “latest response wins” semantics. |
internal/api/client.go |
Wraps both standard and streaming HTTP doers with the version protocol wrapper in NewClient. |
internal/api/client_test.go |
Asserts NewClient sends VOL-180 headers and records instruction headers end-to-end. |
internal/cmd/upgrade/upgrade.go |
Removes GitHub-based polling notice code and adds PrintAPIInstructionNotices + new notice renderers. |
internal/cmd/upgrade/upgrade_test.go |
Replaces cache/polling tests with instruction-driven notice tests using a real API client call. |
internal/cmd/root/root.go |
Removes the prior PersistentPreRun update notice hook. |
cmd/volcano/main.go |
Prints API-driven notices post-Execute(), enriches 426 errors with latest-version target, and adds reauth hinting. |
cmd/volcano/main_test.go |
Tests printDeprecationError and printError behaviors driven by recorded instruction headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tkkhq
added a commit
that referenced
this pull request
Jul 9, 2026
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.
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/<version> (<os>/<arch>), 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).
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.
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.
…tion (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.
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.
) 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.
tkkhq
force-pushed
the
feat/api-version-protocol
branch
from
0a6894a to
8fd1132
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the GitHub-release-based update-notice check (VOL-168) with the Volcano Hosting API as the source of truth for CLI version signaling, per VOL-180. The CLI now reports its own version on every API request and acts on whatever instruction the API returns, instead of independently polling GitHub on every command invocation.
Server counterpart: volcano-hosting#522 (gate + telemetry, merge-ready) and volcano-hosting#532 (dormant policy-arming, do-not-merge-yet draft). This PR is code-complete and does not depend on those being merged first — the protocol is additive and the server ships the gate dormant either way.
Changes
internal/api: aversionProtocolDoerwraps every HTTP call the generated client makes (including unauthenticated ones like device/token exchange) to:X-Volcano-CLI-Versionand a realUser-Agent: volcano-cli/<version> (<os>/<arch>), replacing Go's defaultGo-http-clientUA.X-Volcano-CLI-Instruction/X-Volcano-CLI-Latest-Version/X-Volcano-Device-Instructionresponse headers via the newapi.LastInstructions(). One CLI process runs exactly one command, so this is safe as mutex-guarded process-global state.internal/cmd/upgrade: removed the GitHub-polling notice machinery (MaybePrintUpdateNotice+ its on-disk cache) entirely. AddedPrintAPIInstructionNotices, which renders a non-blocking upgrade suggestion or a deprecation warning straight fromapi.LastInstructions()— zero extra network round-trips, since it only reads headers the command's own API calls already received (previously: up to a 2s GitHub call before every command even started, unless cached).volcano upgrade's actual download/checksum/cosign-verify flow (internal/update) is unchanged — GitHub remains the download/release-artifact source; only the "should I nudge/require an upgrade" decision moves to the API.cmd/volcano/main.go: prints notices unconditionally afterExecute()returns (cobra skipsPersistentPostRunwhenRunEerrors, so this can't live there or a 426/exempt-route case would be missed). A426 require_version_upgradeerror gets the concrete upgrade target appended when the API provided one; any other error gets arun volcano loginhint when the API signaleddevice_instruction=reauth.Security model
The claimed version has no cryptographic binding to a real release — volcano-cli is open source, so per-version keypairs/checksums for client attestation were considered and rejected as unwinnable (see
docs/cli/version-gating.mdin volcano-hosting). This is intentional: the header is advisory input the CLI cooperates with, never a privilege — spoofing it can only ever escape a restriction (run an old/fake version, get told to upgrade less), never escalate one. All authority stays in the bearer credential.Reserved for future work
CLIInstructionLowCreditWarning/CLIInstructionNotEnoughCredit(low_credit_warning/not_enough_credit) are wired intoPrintAPIInstructionNotices— real, tested cases, not just reserved constants — per review discussion on volcano-hosting#522. Unreached today: the server never emits them (see volcano-hosting's matchingcreditgateno-op scaffold), so 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 — needs a target (URL? command?) that doesn't exist yet.
internal/confirmalready has the prompt primitive for that once there's something concrete to confirm. Billing lives in a separate service, so the real gate + this CLI's interactive response to it needs its own design pass once that integration is scoped.Test
internal/api:versionProtocolDoersets both headers, is a no-op on transport error, and merges per-field rather than overwriting — an earlier real instruction survives a later response that simply doesn't repeat the header (a genuine bug caught in review); full round-trip throughNewClient+ a real generated-client call.internal/cmd/upgrade: suggestion, deprecation, and the reserved credit-instruction cases, white-labelCommandPathPrefixrendering — all driven through a realapi.Clientcall against a test server.cmd/volcano:run()itself is tested (not just the extracted print helpers) — 426 short-circuit without duplicating the notice, error-before-notice ordering, reauth ordering, and the deprecation notice on a successful/exempt-route response.go build ./...,go test ./... -count=1,-count=5 -shuffle=onon the touched packages,make lint(0 issues),make tidy(no go.mod/go.sum diff).