feat(cli): avocado login as a shortcut for connect auth login - #242
feat(cli): avocado login as a shortcut for connect auth login#242mobileoverlord wants to merge 1 commit into
avocado login as a shortcut for connect auth login#242Conversation
There was a problem hiding this comment.
🟡 Changes recommended
LoginArgs::run awaits an async method that borrows a temporary ConnectAuthLoginCommand, which will fail to compile due to a temporary dropped while borrowed across .await.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a top-level avocado login command as a shortcut for avocado connect auth login, implemented by sharing a single LoginArgs struct and dispatch path so the two spellings can’t drift.
Changes:
- Introduces
Commands::Login(LoginArgs)as a top-level CLI verb with help text explaining why login matters for feed access. - Refactors
connect auth loginto reuse the sameLoginArgsand execution path (LoginArgs::run()). - Adds a parse/routing regression test ensuring both spellings parse equivalently and neither triggers VM routing.
File summaries
| File | Description |
|---|---|
| src/main.rs | Adds the new top-level login command, shares args with connect auth login, and adds a regression test for parsing + VM routing. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The new regression test doesn’t fully assert equivalence for all parsed LoginArgs fields (e.g., url and output), weakening the stated guarantee that the two spellings cannot diverge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new regression test does not fully enforce “no drift” between the two spellings (it should compare all shared parsed fields), and there’s a small return-type consistency issue in LoginArgs::run.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/main.rs:5294
- This test aims to ensure
avocado loginandconnect auth logincannot drift, but it only validatestoken,profile, andorgagainst constants. To actually guard against drift, compare all shared parsed fields between the two (url,token,profile,org,output), and then assert the expected values on just one side.
for args in [a, b] {
assert_eq!(
(
args.token.as_deref(),
args.profile.as_deref(),
args.org.as_deref()
),
(Some("t"), Some("p"), Some("o"))
);
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The change is a small, well-scoped CLI refactor with a targeted regression test ensuring the shortcut and nested command stay equivalent and remain non-routed.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new regression test does not yet assert all shared LoginArgs fields (e.g., url and output), so the “cannot diverge” guarantee described in the PR can still regress without failing tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/main.rs:5290
- This test claims to ensure
avocado loginandconnect auth logincannot diverge, but it only checkstoken/profile/org. It should also assert the remaining shared fields (urlandoutput) to match expectations, otherwise drift in those flags or defaults won’t be caught.
for args in [a, b] {
assert_eq!(
(
args.token.as_deref(),
args.profile.as_deref(),
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, behavior-preserving (shared args + shared execution), and includes a focused regression test confirming equivalence and VM-routing expectations.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/main.rs:1449
- Use "Log in" (verb) rather than "Login" (noun/adjective) for consistency with the new top-level
loginhelp text and standard grammar.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The shortcut reuses a single args struct and execution path, and the added test covers both CLI spellings plus the VM-routing invariant.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
79ed664 to
2f8334d
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, keeps behavior unified via a shared args struct + shared execution path, and includes a targeted regression test for equivalence and VM-routing invariants.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
2f8334d to
b197c86
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The change is small and well-contained, reuses a shared args struct to prevent drift, and includes a targeted regression test for the key behavior and VM-routing constraint.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
b197c86 to
bee387d
Compare
Logging in is what raises the package-feed rate limit and gives access to private feeds, so it deserves a top-level verb rather than being three words deep. Both spellings share one `LoginArgs` struct and run the same command, so flags, help text and behaviour cannot drift; the nested variant becomes a tuple variant over that struct. Neither routes to the avocado-vm: they are pure Connect API calls, and routing could auto-start a virtual machine for a login. The regression test asserts all five shared fields rather than a sample — the whole point is that the two cannot diverge, and a test covering three of five would not notice the other two diverging. Review also noted that this file aliases `anyhow::Result` as `Result` everywhere else including `main`, so the one `anyhow::Result` spelling invited the reader to look for a difference that was not there. One review finding was answered rather than changed: the claim that borrowing a temporary across the `.await` cannot compile. A temporary lives to the end of its enclosing statement, so it does; the build and the full suite confirm it.
bee387d to
f51e935
Compare
Adds
avocado loginas a top-level shortcut foravocado connect auth login.Logging in is what raises the package-feed rate limit and unlocks private feeds, so it deserves a top-level verb rather than being three words deep.
Both spellings share one
LoginArgsstruct and run the same command, so flags, help text and behaviour cannot drift; the nested variant becomes a tuple variant over that struct.-pand the other short flags are untouched.Neither spelling routes to the avocado-vm: they are pure Connect API calls, and routing could auto-start a virtual machine for a login.
How it was tested
A parse test asserts that
avocado loginandavocado connect auth loginproduce the same arguments from the same flags, and that VM routing is off for both. That is the regression that matters, since the whole point is that the two cannot diverge.Stacked on #241, which it does not depend on functionally — it is based there only to keep the branch history linear. Review it on its own; the diff is one file.