feat(cli): enhance welcome flow, enforce auth, and customize help output#95
feat(cli): enhance welcome flow, enforce auth, and customize help output#95omsherikar merged 4 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds device-code authentication, local credential storage, API key validation, expanded CLI commands/UI (login, auth status/logout, custom help), device authorization/token polling modules, tests for login flows, and version/metadata updates. Includes CLI theming and startup/dashboard UX changes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant API as "Refactron API"
participant Storage as "Local Storage"
User->>CLI: refactron login
CLI->>API: POST /oauth/device
API-->>CLI: DeviceAuthorization (device_code, user_code, verification_uri)
CLI->>User: display verification_uri and user_code
User->>API: visit verification_uri and enter user_code
loop poll until success or expiry
CLI->>API: POST /oauth/token (poll with device_code)
API-->>CLI: authorization_pending / slow_down / error / success
CLI->>CLI: adjust interval on slow_down
end
API-->>CLI: TokenResponse (access_token, email, plan, api_key?)
CLI->>User: prompt for API key (optional) / validate
CLI->>Storage: save_credentials(RefactronCredentials)
Storage-->>CLI: persisted (~/.refactron/credentials.json)
CLI->>User: display success (email, plan)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@refactron/cli.py`:
- Around line 738-776: The global auth gate currently exempts only "login" and
"logout" via the exempt_commands list, which blocks the "auth" command group (so
subcommands like "auth status" can't run); update the exempt_commands
declaration (used where ctx.invoked_subcommand is checked) to include "auth" so
that the auth command group and its subcommands (e.g., status/logout) are
allowed to run without prior authentication; no other logic changes to
load_credentials, ctx.invoke(login,...), or the post-login re-check are needed.
- Around line 835-841: Replace the hard-coded app URL construction assigned to
login_url with the server-provided device verification URI: use
auth.verification_uri (and if you want a direct link, append the code as a query
param or use auth.verification_uri_complete if available) instead of building
f"https://app.refactron.dev/…"; update the displayed URL and keep the displayed
verification code using auth.user_code so the Text() instructions (variable
instructions) correctly reflect the server-supplied verification URI for
non-production API bases.
In `@refactron/core/credentials.py`:
- Around line 43-48: The parsing of expires_at in load_credentials currently
calls datetime.fromisoformat(expires_at_raw) which raises ValueError on
malformed ISO strings; update the block handling expires_at_raw to catch
exceptions (ValueError) around datetime.fromisoformat so that on any parse error
expires_at stays None (preserving the “returns None if invalid” contract), and
keep the existing timezone fix-up (replace tzinfo=timezone.utc) only when
parsing succeeds; reference symbols: expires_at_raw, expires_at,
load_credentials, datetime.fromisoformat.
In `@tests/test_cli_integration.py`:
- Around line 1-2: Remove the stray blank lines in the test file so it is not
entirely empty—either delete the file if unused or replace the blank-only
contents with the minimal valid file (e.g., an empty Python file containing a
single newline) and re-run the formatters (Black / end-of-file-fixer) to ensure
formatting hooks pass; target the test_cli_integration.py test module for this
change.
In `@tests/test_cli_login.py`:
- Around line 19-27: The test asserts the API base URL inside
_mock_start_device_authorization but the CLI call on lines ~75 omits
--api-base-url and the expected output is hard-coded; update the test invocation
that calls the login CLI to pass --api-base-url "http://0.0.0.0:3001" and change
the output assertion to assert against the verification_uri from the mocked
DeviceAuthorization (the "verification_uri" value returned by
_mock_start_device_authorization) instead of a hard-coded host string so the
test aligns with the mock.
- Around line 136-140: The test assertions expect the old success message and
plan but the CLI now prints a fast-path message "Already authenticated" and only
includes the user email; update the test in tests/test_cli_login.py (the block
invoking runner.invoke(main, ["login", "--no-browser"]) and checking
result.output) to assert that result.exit_code == 0, assert "Already
authenticated" (or allow either "Logged in" OR "Already authenticated") is in
result.output, assert "existing@example.com" is in result.output, and remove (or
make optional) the assertion that "pro" is in result.output so the test matches
the current fast-path output from main.
| login_url = f"https://app.refactron.dev/login?{urlencode({'code': auth.user_code})}" | ||
|
|
||
| instructions = Text() | ||
| instructions.append("Please visit the following URL to authenticate:\n\n", style="dim") | ||
| instructions.append(f" {login_url}\n\n", style="underline bold #5f5fff") | ||
| instructions.append("Verification Code:\n", style="dim") | ||
| instructions.append(f" {auth.user_code}\n", style="bold white") |
There was a problem hiding this comment.
Use the device authorization verification_uri instead of a hard-coded app URL.
Line 835 builds a fixed login URL, which breaks when --api-base-url points to staging/local and ignores the server-provided verification URI. Use auth.verification_uri (optionally appending the code) so the flow works across environments.
🔧 Proposed fix
- login_url = f"https://app.refactron.dev/login?{urlencode({'code': auth.user_code})}"
+ login_url = auth.verification_uri
+ if auth.user_code:
+ sep = "&" if "?" in login_url else "?"
+ login_url = f"{login_url}{sep}{urlencode({'code': auth.user_code})}"🤖 Prompt for AI Agents
In `@refactron/cli.py` around lines 835 - 841, Replace the hard-coded app URL
construction assigned to login_url with the server-provided device verification
URI: use auth.verification_uri (and if you want a direct link, append the code
as a query param or use auth.verification_uri_complete if available) instead of
building f"https://app.refactron.dev/…"; update the displayed URL and keep the
displayed verification code using auth.user_code so the Text() instructions
(variable instructions) correctly reflect the server-supplied verification URI
for non-production API bases.
| expires_at_raw = data.get("expires_at") | ||
| expires_at: Optional[datetime] = None | ||
| if isinstance(expires_at_raw, str) and expires_at_raw.strip(): | ||
| expires_at = datetime.fromisoformat(expires_at_raw) | ||
| if expires_at.tzinfo is None: | ||
| expires_at = expires_at.replace(tzinfo=timezone.utc) |
There was a problem hiding this comment.
Guard expires_at parsing against invalid ISO strings.
Line 46 can raise ValueError on malformed timestamps, which bubbles up and contradicts the “returns None if invalid” contract in load_credentials. Treat invalid values as None so the CLI doesn’t crash on a corrupted credentials file.
🔧 Proposed fix
- if isinstance(expires_at_raw, str) and expires_at_raw.strip():
- expires_at = datetime.fromisoformat(expires_at_raw)
- if expires_at.tzinfo is None:
- expires_at = expires_at.replace(tzinfo=timezone.utc)
+ if isinstance(expires_at_raw, str) and expires_at_raw.strip():
+ try:
+ expires_at = datetime.fromisoformat(expires_at_raw)
+ except ValueError:
+ expires_at = None
+ else:
+ if expires_at.tzinfo is None:
+ expires_at = expires_at.replace(tzinfo=timezone.utc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expires_at_raw = data.get("expires_at") | |
| expires_at: Optional[datetime] = None | |
| if isinstance(expires_at_raw, str) and expires_at_raw.strip(): | |
| expires_at = datetime.fromisoformat(expires_at_raw) | |
| if expires_at.tzinfo is None: | |
| expires_at = expires_at.replace(tzinfo=timezone.utc) | |
| expires_at_raw = data.get("expires_at") | |
| expires_at: Optional[datetime] = None | |
| if isinstance(expires_at_raw, str) and expires_at_raw.strip(): | |
| try: | |
| expires_at = datetime.fromisoformat(expires_at_raw) | |
| except ValueError: | |
| expires_at = None | |
| else: | |
| if expires_at.tzinfo is None: | |
| expires_at = expires_at.replace(tzinfo=timezone.utc) |
🤖 Prompt for AI Agents
In `@refactron/core/credentials.py` around lines 43 - 48, The parsing of
expires_at in load_credentials currently calls
datetime.fromisoformat(expires_at_raw) which raises ValueError on malformed ISO
strings; update the block handling expires_at_raw to catch exceptions
(ValueError) around datetime.fromisoformat so that on any parse error expires_at
stays None (preserving the “returns None if invalid” contract), and keep the
existing timezone fix-up (replace tzinfo=timezone.utc) only when parsing
succeeds; reference symbols: expires_at_raw, expires_at, load_credentials,
datetime.fromisoformat.
| def _mock_start_device_authorization(api_base_url: str, timeout_seconds: int = 10): | ||
| assert api_base_url == "http://0.0.0.0:3001" | ||
| return DeviceAuthorization( | ||
| device_code="devcode-123", | ||
| user_code="ABCD-EFGH", | ||
| verification_uri="https://refactron.dev/auth/device", | ||
| expires_in=900, | ||
| interval=1, | ||
| ) |
There was a problem hiding this comment.
Pass --api-base-url and align the login URL with the mock response.
Line 19 asserts a local base URL, but Line 75 invokes login without --api-base-url, so it defaults to the production value. Also, the output assertion should track the verification URL returned by the mock to avoid hard-coded host coupling.
🔧 Proposed fix
- return DeviceAuthorization(
+ return DeviceAuthorization(
device_code="devcode-123",
user_code="ABCD-EFGH",
- verification_uri="https://refactron.dev/auth/device",
+ verification_uri="http://localhost:3000/login",
expires_in=900,
interval=1,
)
@@
- result = runner.invoke(main, ["login", "--no-browser"])
+ result = runner.invoke(
+ main, ["login", "--no-browser", "--api-base-url", "http://0.0.0.0:3001"]
+ )
@@
- assert "localhost:3000/login" in result.output
+ assert "localhost:3000/login" in result.outputAlso applies to: 75-81
🧰 Tools
🪛 Ruff (0.14.14)
19-19: Unused function argument: timeout_seconds
(ARG001)
🤖 Prompt for AI Agents
In `@tests/test_cli_login.py` around lines 19 - 27, The test asserts the API base
URL inside _mock_start_device_authorization but the CLI call on lines ~75 omits
--api-base-url and the expected output is hard-coded; update the test invocation
that calls the login CLI to pass --api-base-url "http://0.0.0.0:3001" and change
the output assertion to assert against the verification_uri from the mocked
DeviceAuthorization (the "verification_uri" value returned by
_mock_start_device_authorization) instead of a hard-coded host string so the
test aligns with the mock.
There was a problem hiding this comment.
Pull request overview
Adds a device-code login experience to the Refactron CLI, persists credentials locally, enforces authentication before most commands, and replaces the default Click help with a custom Rich-rendered help/welcome flow.
Changes:
- Introduces device authorization + token polling helpers and a local credentials store.
- Adds CLI
login/logout/auth statusand a global auth gate, plus a new Rich-themed welcome/help UX. - Adds pytest coverage for login/logout/auth-status flows (with one placeholder integration test file).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
refactron/cli.py |
Adds auth gating, login/logout/auth commands, API key verification, and new Rich UI/help/welcome behavior |
refactron/core/device_auth.py |
Implements device-code style auth requests (/oauth/device, /oauth/token) |
refactron/core/credentials.py |
Implements JSON credential persistence with best-effort permission tightening |
tests/test_cli_login.py |
Adds tests for login/logout/auth status behavior |
tests/test_cli_integration.py |
Currently empty placeholder |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from urllib.parse import urlencode | ||
|
|
||
| import click | ||
| import requests # type: ignore |
There was a problem hiding this comment.
requests is imported here but it is not declared in the project dependencies (pyproject.toml / requirements.txt). This will cause a runtime ModuleNotFoundError for users installing the package.
Consider either adding requests to the core dependencies (and updating requirements.txt), or rewriting _validate_api_key to use the existing stdlib HTTP approach used in refactron.core.device_auth to avoid a new dependency.
refactron/cli.py
Outdated
| # Check authentication for all commands except login/logout | ||
| exempt_commands = ["login", "logout"] |
There was a problem hiding this comment.
The global auth gate runs for any ctx.invoked_subcommand not in ['login','logout'], which includes auth status. That makes refactron auth status unusable when logged out (and conflicts with auth_status() explicitly handling the logged-out case).
Please exempt auth status (or the whole auth group) from the auth gate, or move auth enforcement to only the commands that truly require it.
| # Check authentication for all commands except login/logout | |
| exempt_commands = ["login", "logout"] | |
| # Check authentication for all commands except login/logout/auth | |
| exempt_commands = ["login", "logout", "auth"] |
| console.print(Align.center(Text("\nAuthentication Required", style="bold"))) | ||
|
|
||
| if Prompt.ask("\nLog in to continue?", choices=["y", "n"], default="y") == "y": | ||
| try: | ||
| ctx.invoke( |
There was a problem hiding this comment.
This interactive prompt (Prompt.ask) will block or raise EOFError in non-interactive contexts (CI, scripts, CliRunner tests) when credentials are missing.
Consider detecting non-TTY input (or a --non-interactive/--yes flag) and failing fast with a clear message instead of prompting, so CLI commands behave predictably in automation.
| if ctx.invoked_subcommand is None: | ||
| _run_startup_animation() | ||
| _run_minimal_loop(ctx) |
There was a problem hiding this comment.
Running the CLI with no subcommand now triggers an animation and then enters an infinite interactive loop, which is a breaking change from typical CLI behavior (it will hang in scripts/CI when refactron is invoked without args).
Consider only entering this mode when stdin/stdout are TTYs, and otherwise default to showing help and exiting non-interactively.
| def _mock_start_device_authorization(api_base_url: str, timeout_seconds: int = 10): | ||
| assert api_base_url == "http://0.0.0.0:3001" | ||
| return DeviceAuthorization( |
There was a problem hiding this comment.
This mock asserts api_base_url == 'http://0.0.0.0:3001', but refactron login defaults --api-base-url to DEFAULT_API_BASE_URL (https://api.refactron.dev). As written, the test will fail unless it passes --api-base-url http://0.0.0.0:3001 (or the CLI default is changed).
Update the test invocation to include --api-base-url or relax the assertion to match the current CLI behavior.
| creds = load_credentials() | ||
| if creds and creds.access_token: | ||
| is_authenticated = True | ||
| except SystemExit: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except SystemExit: | |
| except SystemExit: | |
| # User aborted the login flow (e.g. via Ctrl+C or an explicit exit); | |
| # treat this as "not authenticated" and fall through to the normal exit path. |
| # Best-effort permissions tightening (Windows may ignore chmod semantics). | ||
| try: | ||
| os.chmod(target, 0o600) | ||
| except OSError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except OSError: | |
| except OSError: | |
| # Intentionally ignore permission errors: some platforms or filesystems | |
| # may not support chmod semantics, but this should not block saving creds. |
| data = json.loads(raw) if raw else {} | ||
| if isinstance(data, dict): | ||
| return data | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # If the error body cannot be parsed as JSON for any reason, | |
| # ignore the parsing failure and fall back to a generic error. |
| if ctx.invoked_subcommand is None: | ||
| _run_startup_animation() | ||
| _run_minimal_loop(ctx) | ||
| pass |
There was a problem hiding this comment.
Unnecessary 'pass' statement.
| pass |
github-actions
bot
added
dependencies
documentation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.