Skip to content

feat(cli): avocado login as a shortcut for connect auth login - #242

Open
mobileoverlord wants to merge 1 commit into
jschneck/feeds-namedfrom
jschneck/cli-login
Open

feat(cli): avocado login as a shortcut for connect auth login#242
mobileoverlord wants to merge 1 commit into
jschneck/feeds-namedfrom
jschneck/cli-login

Conversation

@mobileoverlord

@mobileoverlord mobileoverlord commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Owner: the secure-feeds series (241 → 242 → 243 → 244). Both sessions push as mobileoverlord, so GitHub cannot tell you which one wrote a PR — this line can.

Adds avocado login as a top-level shortcut for avocado 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 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. -p and 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 login and avocado connect auth login produce 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 login to reuse the same LoginArgs and 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.

Comment thread src/main.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread src/main.rs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 login and connect auth login cannot drift, but it only validates token, profile, and org against 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

Comment thread src/main.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 login and connect auth login cannot diverge, but it only checks token/profile/org. It should also assert the remaining shared fields (url and output) 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

Comment thread src/main.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 login help text and standard grammar.
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants