Repository navigation
fix(cycles): icp cycles buy requires one of --amount/--cycles and refuses negatives - #826
Merged
Merged
Conversation
lwshang
approved these changes
Oct 19, 2026
raymondk
force-pushed
the
cycles-buy/02-arg-group
branch
from
October 20, 2026 15:45
f5488ad to
c249774
Compare
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Static review found no blocking issues, and targeted tests cover the intended behavior; tests were not executed.
No open issues 🎉
What changed in this PR
Improves icp cycles buy argument validation so missing purchase options and negative values produce clear, consistent errors.
Changes:
- Requires exactly one of
--amountor--cycles. - Rejects negative values during argument parsing with exit status 2.
- Adds regression coverage and updates documentation.
| File | Description |
|---|---|
docs/reference/cli.md |
Updates usage to show the required alternatives. |
crates/icp-cli/tests/cycles_tests.rs |
Extends argument-validation integration coverage. |
crates/icp-cli/src/commands/cycles/buy.rs |
Adds argument grouping, sign validation, and unit tests. |
CHANGELOG.md |
Records the improved validation behavior. |
Review effort: Balanced
…uses negatives
Before, `icp cycles buy` with neither flag said only that --amount was
required, as if --cycles were not a way in; and a negative value of
either was read by clap as a flag nobody passed (`-5t` was "unexpected
argument '-5t'"), with no word about the sign. Now a clap ArgGroup
names both flags as the one required argument, and the two options
take a leading `-` as part of their value and refuse it in words
("cannot be negative: give the cycles to receive, such as 5t"), with
exit status 2 for both.
Extracted from PR #824.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Twxau844tMTwyHpQJEbmBo
raymondk
force-pushed
the
cycles-buy/02-arg-group
branch
from
October 20, 2026 17:45
c249774 to
2e2fd03
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.
With neither
--amountnor--cycles,icp cycles buysaid only that--amountwas required, as if--cycleswere not a way in; and a negative value of either was read by clap as a flag nobody passed (-5t→ "unexpected argument"), with no word about the sign.Now a clap
ArgGroupnames both flags as the one required argument, and both options take a leading-as part of their value and refuse it in words ("cannot be negative: give the cycles to receive, such as 5t"), exit status 2 for both.Code is Josh's, verbatim from #824 (commit 18f7794, argument-parsing part only).
cli.mdregenerated (usage line). Unit testamount_or_cycles_is_required_and_neither_may_be_negativeand the extended integration testcycles_buy_rejects_conflicting_arguments.Part of restacking #824 into small PRs, one fix each, stacked on
mainin this order:cycles-buy/01-json-implies-yes→02-arg-group→03-gateway→04-qr-colours→05-paste-flags→ then the wait, its follow-up text and the--jsonfields. Each PR targets the one below it; after the one below merges, retarget tomain.🤖 Generated with Claude Code
https://claude.ai/code/session_01Twxau844tMTwyHpQJEbmBo