feat(upgrade): render credit notices with billing URL (VOL-354) - #183
Open
tkkhq wants to merge 4 commits into
Open
feat(upgrade): render credit notices with billing URL (VOL-354)#183tkkhq wants to merge 4 commits into
tkkhq wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Document the new user-facing behavior and update the stale renderer comment before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds account-credit notices and API-provided billing URLs to the CLI instruction protocol.
Changes:
- Tracks billing URLs alongside credit instructions.
- Renders styled low-credit and read-only notices.
- Tests URL persistence, updates, and terminal output.
File summaries
| File | Description |
|---|---|
internal/cmd/upgrade/upgrade.go |
Renders credit notices and billing links. |
internal/cmd/upgrade/upgrade_test.go |
Tests credit-notice output and URL changes. |
internal/api/version_protocol.go |
Tracks billing URLs in instruction state. |
internal/api/version_protocol_test.go |
Tests URL recording, persistence, and consumption. |
Review details
Suppressed comments (1)
internal/cmd/upgrade/upgrade.go:63
- The function's doc comment still says this renderer only handles upgrade suggestions or deprecation warnings and references
LastInstructions, but these branches add credit notices and the implementation consumes them throughConsumeCLIInstructions. Update the comment so the documented behavior matches the expanded function.
case api.CLIInstructionNotEnoughCredit:
printNotEnoughCreditWarning(cmd, instructions.CreditURL)
case api.CLIInstructionLowCreditWarning:
printLowCreditWarning(cmd, instructions.CreditURL)
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+100
to
+101
| fmt.Fprintln(w, theme.Error("Your account is read-only:", on), "not enough credit to complete this request.") | ||
| printCreditURL(w, on, creditURL) |
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.
Tracking
Why
low_credit_warning/not_enough_creditwire values and wired unreachable placeholder cases intoPrintAPIInstructionNotices, but the printed text was a stub and the API's billing URL had nowhere to landSummary
X-Volcano-Credit-URLto the VOL-180 instruction protocol ininternal/api/version_protocol.go: recorded and cleared in lockstep withCLIInstruction/LatestVersion(sticky pairing — an unrelated later response never drops an earlier real URL)low_credit_warningas a non-blockingtheme.Warnnotice andnot_enough_creditas atheme.Errornotice explaining the account is read-only, both ininternal/cmd/upgrade/upgrade.go; print the billing URL (theme.Command) underneath when the API supplied onenot_enough_creditnon-gating on the CLI side — the API's own error response remains the actual blocker; this notice only adds context, printed the same way the existing upgrade/deprecation notices areCreditURLinConsumeCLIInstructions's consumed-notice identity (cliInstructionPair) so a long-running command (e.g. a log-follow) that consumes alow_credit_warning/not_enough_creditnotice, then observes the same instruction and latest version again but with a changed credit URL, renders the updated URL instead of suppressing it as an already-consumed repeat; version-only suggestion/deprecation behavior is unchanged since those instructions never carry a credit URL"your account is running low on credit.") — the Hosting credit signal is stored per account/user, not per projectinternal/confirmremains the primitive for that once there's a real targetCLIInstructionLowCreditWarning/CLIInstructionNotEnoughCreditto describe the account's credit balance rather than the project'sdocs/README.mdPreview
Verification
go build ./...go test ./... -count=1go test ./internal/api/... ./internal/cmd/upgrade/... ./cmd/volcano/... -count=5 -shuffle=onmake lintmake tidy(nogo.mod/go.sumdiff)make buildmake check