Refactor the CLI into a Typer command package and improve session handling#203
Merged
timlaing merged 20 commits intotimlaing:mainfrom Mar 22, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Typer-based CLI with many subcommands, a shared CLI state/options system, account discovery persistence, output/normalization helpers, extended PyiCloudService session/auth management (resolve_cookie_directory, authenticate flag, get_auth_status, logout, clear state), session persistence helpers, tests, and packaging/README updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as Typer CLI
participant State as CLIState
participant Keyring as Keyring
participant API as PyiCloudService
participant Accounts as Account Index
User->>CLI: icloud auth login
CLI->>State: get_state(from_options)
State->>State: _resolve_username()
State->>Keyring: has_keyring_password(username)
alt keyring has password
Keyring-->>State: password
else no password
State->>User: prompt for password
User-->>State: password
end
State->>API: build_session_api() / get_login_api()
API->>API: authenticate (may require 2FA/2SA)
alt 2FA/2SA required
API-->>CLI: request code / device list
CLI->>User: prompt for code/selection
User-->>CLI: code/selection
CLI->>API: submit code/selection
end
API-->>State: authenticated
State->>Accounts: remember_account(api)
Accounts->>Accounts: upsert & save `accounts.json`
CLI-->>User: login success
sequenceDiagram
participant User as User
participant CLI as Typer CLI
participant State as CLIState
participant Storage as Local files
participant API as PyiCloudService
User->>CLI: icloud account summary
CLI->>State: get_state(from_options)
State->>Storage: check session/cookie files & account index
alt session present or keyring password
State->>API: build_session_api()
API-->>State: restored session
State->>API: service_call("Account", api.account)
API-->>State: account data
CLI->>CLI: normalize_account_summary()
CLI-->>User: display summary (JSON or table)
else no session
CLI-->>User: not logged in message / abort
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
tests/test_cmdline.py (1)
23-23: Consider usingtmp_pathfixture ortempfilefor test isolation.The hardcoded path
/tmp/python-test-results/test_cmdlinemay cause issues on non-Unix systems (e.g., Windows) and could lead to test pollution if tests run concurrently. Using pytest'stmp_pathfixture would provide better isolation and cross-platform compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cmdline.py` at line 23, The test uses a hardcoded TEST_ROOT = Path("/tmp/python-test-results/test_cmdline") which breaks portability and isolation; change the tests to use pytest's tmp_path fixture (or tempfile.TemporaryDirectory) instead: remove or stop relying on the global TEST_ROOT and refactor the tests in tests/test_cmdline.py (references to TEST_ROOT) to accept tmp_path as a function argument and create needed directories/files under tmp_path so each test gets an isolated, cross-platform temporary directory.README.md (1)
174-177: Minor grammar improvement.Consider adding a comma after "Alternatively" for improved readability.
📝 Suggested change
-using `icloud auth login --accept-terms`. Alternatively you can visit +using `icloud auth login --accept-terms`. Alternatively, you can visit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 174 - 177, The sentence "Alternatively you can visit the iCloud web site to view and accept the terms." needs a comma after "Alternatively" for readability; update the README sentence to "Alternatively, you can visit the iCloud web site to view and accept the terms." (search for the exact string in README.md to locate and replace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyicloud/cli/account_index.py`:
- Around line 33-38: The current load/write cycle can corrupt accounts.json
because _save_accounts() overwrites in-place; change _save_accounts() to write
JSON to a temporary file in the same directory (use tempfile.NamedTemporaryFile
or a .tmp file) and then atomically replace the target with
os.replace(temp_path, index_path) so interrupted writes never leave invalid
JSON; also serialize the update path in remember_account() (and any other code
paths that call _save_accounts(), e.g., the blocks around lines 65–85 and
125–140) by using an inter-process file lock (fcntl or a small
lockfile/portalocker) to prevent concurrent CLI processes from stomping each
other.
In `@pyicloud/cli/commands/devices.py`:
- Around line 113-115: Mutating Find My calls (idevice.play_sound,
idevice.display_message, idevice.lost_device, idevice.erase_device) must be
invoked inside the session-safe wrapper service_call to ensure re-authentication
and proper CLI error handling; for each occurrence (e.g., after resolve_device
returns idevice) replace the direct call like
idevice.play_sound(subject=subject) with a call through service_call (for
example: service_call(lambda: idevice.play_sound(subject=subject)) or the
project's established service_call invocation style), keeping the same payload
construction (payload = {"device_id": idevice.id, "subject": subject}) and apply
the same pattern to the blocks around lines cited (play_sound, display_message,
lost_device, erase_device).
- Around line 166-167: resolve_device currently can return a non-unique match so
destructive commands call idevice.lost_device and idevice.erase on the wrong
device; before calling idevice.lost_device(...) and idevice.erase(...) ensure
the resolved device is an unambiguous, unique match (or require an explicit
device id). Update the call site in devices.py to validate the result of
resolve_device(api, device): if resolve_device indicates multiple matches (or
returns more than one candidate) raise an error asking the user to specify the
device id (or disallow the operation), otherwise proceed to call
idevice.lost_device(...) / idevice.erase(...); apply the same uniqueness check
to the other occurrence referenced (lines ~194-195).
In `@pyicloud/cli/commands/drive.py`:
- Around line 75-98: The drive_download command calls resolve_drive_node without
handling KeyError, so missing paths will raise an unhandled exception; wrap the
resolve_drive_node(...) call in a try/except KeyError block inside
drive_download, catch the KeyError and raise CLIAbort with a clear message
(e.g., "Drive path not found" including the path), preserving existing behavior
for state.json_output and state.console.print; ensure you reference
resolve_drive_node and drive_download when making this change.
- Line 37: resolve_drive_node can raise KeyError for missing path components
which currently bubbles up as a traceback; wrap the call(s) to
resolve_drive_node in drive.py (the invocation at the node =
resolve_drive_node(drive, path, trash=trash) and the similar call in
drive_download) with a try/except KeyError block and raise a CLIAbort with a
clear user-facing message (e.g. "Path not found: <path>") including the original
path variable so users see a friendly error instead of a traceback; reference
resolve_drive_node, drive_download and the local variable path to locate where
to add the handler.
In `@pyicloud/cli/commands/photos.py`:
- Around line 24-27: The current service_call("Photos", lambda: api.photos) only
protects creating the lazy Photos client; move the actual RPC usages inside
service_call so session re-auth failures get handled: replace calls that
reference photos.albums, album_obj.photos, photos.all[photo_id], and
photo.download() with service_call invocations that perform the real RPC work
(for example, wrap the list comprehension that calls normalize_album over
api.photos.albums inside service_call, and similarly wrap fetching
album_obj.photos, accessing photos.all[...] and downloading a photo inside
service_call lambdas). Update every place where those symbols are accessed (the
other occurrences that use photos.albums / album_obj.photos / photos.all /
photo.download) to follow the same pattern.
In `@pyicloud/cli/context.py`:
- Around line 341-363: In _handle_2sa, move the interactive-mode check to before
the call to api.send_verification_code so we don't send a verification code when
prompts are disabled; specifically, check self.interactive right after selecting
the device (before invoking api.send_verification_code(device)) and raise
CLIAbort("Two-step authentication is required, but interactive prompts are
disabled.") if false, keeping the existing flow that then calls
api.send_verification_code, uses typer.prompt to read the code, and calls
api.validate_verification_code.
In `@pyicloud/session.py`:
- Around line 117-120: The current cookie-clear in pyicloud.session.py silently
swallows failures (cast(PyiCloudCookieJar, self.cookies).clear()) which can
leave stale in-memory auth cookies; modify the except block in the method that
calls clear (the cookie-reset/logout routine) to not ignore errors — either log
the exception with context (use logger.exception or processLogger.error with the
exception) and re-raise a specific error, or defensively replace self.cookies
with a fresh PyiCloudCookieJar and persist that replacement; reference the call
site where cast(PyiCloudCookieJar, self.cookies).clear() is used and update it
to log the failure and either raise or recreate the cookie jar so stale cookies
cannot remain.
---
Nitpick comments:
In `@README.md`:
- Around line 174-177: The sentence "Alternatively you can visit the iCloud web
site to view and accept the terms." needs a comma after "Alternatively" for
readability; update the README sentence to "Alternatively, you can visit the
iCloud web site to view and accept the terms." (search for the exact string in
README.md to locate and replace).
In `@tests/test_cmdline.py`:
- Line 23: The test uses a hardcoded TEST_ROOT =
Path("/tmp/python-test-results/test_cmdline") which breaks portability and
isolation; change the tests to use pytest's tmp_path fixture (or
tempfile.TemporaryDirectory) instead: remove or stop relying on the global
TEST_ROOT and refactor the tests in tests/test_cmdline.py (references to
TEST_ROOT) to accept tmp_path as a function argument and create needed
directories/files under tmp_path so each test gets an isolated, cross-platform
temporary directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6be346e-c94d-4875-800a-1772aa1c2be9
📒 Files selected for processing (26)
README.mdpyicloud/base.pypyicloud/cli/__init__.pypyicloud/cli/account_index.pypyicloud/cli/app.pypyicloud/cli/commands/__init__.pypyicloud/cli/commands/account.pypyicloud/cli/commands/auth.pypyicloud/cli/commands/calendar.pypyicloud/cli/commands/contacts.pypyicloud/cli/commands/devices.pypyicloud/cli/commands/drive.pypyicloud/cli/commands/hidemyemail.pypyicloud/cli/commands/photos.pypyicloud/cli/context.pypyicloud/cli/normalize.pypyicloud/cli/options.pypyicloud/cli/output.pypyicloud/cmdline.pypyicloud/session.pypyproject.tomlrequirements.txttests/conftest.pytests/test_base.pytests/test_cmdline.pytests/test_output.py
There was a problem hiding this comment.
🧹 Nitpick comments (6)
pyicloud/cli/commands/calendar.py (1)
78-144: Client-side filtering bycalendar_guidmay be inefficient.The calendar GUID filter at lines 121-124 is applied after fetching all events from the API. For accounts with many events, this fetches more data than necessary. Consider documenting this limitation or checking if the underlying API supports server-side filtering.
Additionally,
isliceis applied after the GUID filter, so thelimitparameter only limits the final output rather than capping the API fetch—this is likely intentional but worth noting in the help text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/commands/calendar.py` around lines 78 - 144, The handler calendar_events applies the calendar_guid filter client-side after fetching events (using service_call -> api.calendar.get_events) and then applies islice(payload, limit) which means API fetches are not capped by limit; update calendar_events to pass calendar_guid into api.calendar.get_events (if that API supports a calendar_guid/calendar argument) so filtering happens server-side and the limit applies to the fetch, otherwise update the CLI help/usage text for the calendar_guid and limit options to explicitly document that filtering is performed client-side and that limit is applied after filtering; adjust references in the code to calendar_guid, service_call/api.calendar.get_events, islice, and limit accordingly.pyicloud/cli/account_index.py (1)
73-91: Lock file is not cleaned up after use.The
.lockfile created at line 80 is never removed, leaving orphan lock files in the session directory. Whilst this is harmless, it may be worth cleaning up the lock file after successful operations or documenting this behaviour.♻️ Optional: Clean up the lock file after use
fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) try: yield index_path finally: fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) + try: + lock_path.unlink(missing_ok=True) + except OSError: + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/account_index.py` around lines 73 - 91, The lock file created in _locked_index (lock_path and lock_file) is never removed; after releasing the fcntl lock in the finally block, attempt to unlink lock_path to clean up orphaned .lock files, but do this safely by catching FileNotFoundError and OSError to avoid raising on concurrent access; ensure you perform the unlink after releasing the lock (and while still inside the with lock_file context or after closing it) so other processes aren’t affected and failures are ignored.pyicloud/cli/commands/hidemyemail.py (2)
103-138: Missingaccount_nameparameter inservice_call.The
service_callwrapper at lines 131-134 omitsaccount_name, which means re-authentication errors will produce a generic message rather than one that identifies the affected account. Other commands in this file have the same omission.♻️ Add account_name for clearer error messages
payload = service_call( "Hide My Email", lambda: api.hidemyemail.reserve(email=email, label=label, note=note), + account_name=api.account_name, )Apply the same fix to
hidemyemail_update,hidemyemail_deactivate,hidemyemail_reactivate, andhidemyemail_delete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/commands/hidemyemail.py` around lines 103 - 138, The service_call invocation in hidemyemail_reserve omits the account_name argument, causing poor re-authentication error messages; update the lambda call to pass account_name=state.username (i.e., service_call("Hide My Email", lambda: api.hidemyemail.reserve(...), account_name=state.username)), and apply the same change to the other functions hidemyemail_update, hidemyemail_deactivate, hidemyemail_reactivate, and hidemyemail_delete so each service_call includes account_name=state.username to surface the affected account in errors.
26-67: Iteration over aliases happens outsideservice_callwrapper.At lines 51-54,
service_callwraps only the access toapi.hidemyemail, but the iteration (list comprehension callingnormalize_alias) happens outside. If the underlying iterator raises an authentication exception during iteration (lazy evaluation), it would bypass theservice_callerror handling and produce a traceback instead of a user-friendly CLI error.Consider wrapping the entire list comprehension:
♻️ Wrap iteration in service_call
- payload = [ - normalize_alias(alias) - for alias in service_call("Hide My Email", lambda: api.hidemyemail) - ] + payload = service_call( + "Hide My Email", + lambda: [normalize_alias(alias) for alias in api.hidemyemail], + account_name=api.account_name, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/commands/hidemyemail.py` around lines 26 - 67, The list comprehension that builds payload in hidemyemail_list is executed outside the service_call wrapper so any lazy-raising errors from api.hidemyemail during iteration bypass service_call’s error handling; fix this by moving the entire iteration into the service_call call (e.g., call service_call("Hide My Email", lambda: [normalize_alias(alias) for alias in api.hidemyemail]) so payload is fully constructed inside service_call), keeping subsequent checks (state.json_output and state.write_json) and console_table rendering the same.tests/test_cmdline.py (1)
24-24: Consider usingtempfilefor cross-platform compatibility.The hardcoded
/tmppath may not exist on Windows systems. While this is test code, usingtempfile.gettempdir()orpytest'stmp_pathfixture would improve portability.♻️ Optional: use tempfile for portability
+import tempfile + -TEST_ROOT = Path("/tmp/python-test-results/test_cmdline") +TEST_ROOT = Path(tempfile.gettempdir()) / "python-test-results" / "test_cmdline"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cmdline.py` at line 24, TEST_ROOT is hardcoded to /tmp which breaks on Windows; replace its construction to use a cross-platform temp dir by importing tempfile and setting TEST_ROOT = Path(tempfile.gettempdir()) / "python-test-results" / "test_cmdline", or alternatively refactor the test to use pytest's tmp_path fixture and remove the global TEST_ROOT; update the reference to TEST_ROOT in tests/test_cmdline.py accordingly so paths are created under a platform-safe temporary directory.pyicloud/cli/commands/devices.py (1)
341-391: Default export behaviour may be unexpected.The logic at line 385 means
--rawis the default when neither flag is provided (raw=Noneandnormalized=False→use_raw=True). This is inconsistent withdevices_showwhich defaults to normalised output when--rawis not specified. Consider aligning the default behaviour or documenting the difference.♻️ Optional: align with devices_show default behaviour
- use_raw = raw is not False and not normalized + use_raw = raw is True # Default to normalized unless --raw is explicitly passed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/commands/devices.py` around lines 341 - 391, The export command currently treats --raw as the default when neither flag is given because use_raw = raw is not False and not normalized; change the logic in devices_export so raw is only used when explicitly passed (e.g. compute use_raw = (raw is True) and not normalized) so the default matches devices_show (normalized output when --raw is not specified) and avoid treating raw=None as true; update the use_raw expression and any related docs or tests if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pyicloud/cli/account_index.py`:
- Around line 73-91: The lock file created in _locked_index (lock_path and
lock_file) is never removed; after releasing the fcntl lock in the finally
block, attempt to unlink lock_path to clean up orphaned .lock files, but do this
safely by catching FileNotFoundError and OSError to avoid raising on concurrent
access; ensure you perform the unlink after releasing the lock (and while still
inside the with lock_file context or after closing it) so other processes aren’t
affected and failures are ignored.
In `@pyicloud/cli/commands/calendar.py`:
- Around line 78-144: The handler calendar_events applies the calendar_guid
filter client-side after fetching events (using service_call ->
api.calendar.get_events) and then applies islice(payload, limit) which means API
fetches are not capped by limit; update calendar_events to pass calendar_guid
into api.calendar.get_events (if that API supports a calendar_guid/calendar
argument) so filtering happens server-side and the limit applies to the fetch,
otherwise update the CLI help/usage text for the calendar_guid and limit options
to explicitly document that filtering is performed client-side and that limit is
applied after filtering; adjust references in the code to calendar_guid,
service_call/api.calendar.get_events, islice, and limit accordingly.
In `@pyicloud/cli/commands/devices.py`:
- Around line 341-391: The export command currently treats --raw as the default
when neither flag is given because use_raw = raw is not False and not
normalized; change the logic in devices_export so raw is only used when
explicitly passed (e.g. compute use_raw = (raw is True) and not normalized) so
the default matches devices_show (normalized output when --raw is not specified)
and avoid treating raw=None as true; update the use_raw expression and any
related docs or tests if needed.
In `@pyicloud/cli/commands/hidemyemail.py`:
- Around line 103-138: The service_call invocation in hidemyemail_reserve omits
the account_name argument, causing poor re-authentication error messages; update
the lambda call to pass account_name=state.username (i.e., service_call("Hide My
Email", lambda: api.hidemyemail.reserve(...), account_name=state.username)), and
apply the same change to the other functions hidemyemail_update,
hidemyemail_deactivate, hidemyemail_reactivate, and hidemyemail_delete so each
service_call includes account_name=state.username to surface the affected
account in errors.
- Around line 26-67: The list comprehension that builds payload in
hidemyemail_list is executed outside the service_call wrapper so any
lazy-raising errors from api.hidemyemail during iteration bypass service_call’s
error handling; fix this by moving the entire iteration into the service_call
call (e.g., call service_call("Hide My Email", lambda: [normalize_alias(alias)
for alias in api.hidemyemail]) so payload is fully constructed inside
service_call), keeping subsequent checks (state.json_output and
state.write_json) and console_table rendering the same.
In `@tests/test_cmdline.py`:
- Line 24: TEST_ROOT is hardcoded to /tmp which breaks on Windows; replace its
construction to use a cross-platform temp dir by importing tempfile and setting
TEST_ROOT = Path(tempfile.gettempdir()) / "python-test-results" /
"test_cmdline", or alternatively refactor the test to use pytest's tmp_path
fixture and remove the global TEST_ROOT; update the reference to TEST_ROOT in
tests/test_cmdline.py accordingly so paths are created under a platform-safe
temporary directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31008416-e956-4547-9130-1c121bdc8cee
📒 Files selected for processing (15)
pyicloud/cli/account_index.pypyicloud/cli/app.pypyicloud/cli/commands/account.pypyicloud/cli/commands/auth.pypyicloud/cli/commands/calendar.pypyicloud/cli/commands/contacts.pypyicloud/cli/commands/devices.pypyicloud/cli/commands/drive.pypyicloud/cli/commands/hidemyemail.pypyicloud/cli/commands/photos.pypyicloud/cli/context.pypyicloud/cli/options.pypyicloud/session.pytests/test_base.pytests/test_cmdline.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyicloud/cli/commands/drive.py
- pyicloud/session.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pyicloud/cli/commands/calendar.py (1)
83-83: Consider restrictingperiodto valid choices.The underlying
get_events()method only handles"day","week", and"month"explicitly. Invalid values would silently fall through with potentially unexpected behaviour. Adding validation would provide clearer feedback to users.♻️ Suggested improvement
- period: str = typer.Option("month", "--period", help="Calendar period shortcut."), + period: str = typer.Option( + "month", + "--period", + help="Calendar period shortcut: day, week, or month.", + click_type=typer.Choice(["day", "week", "month"], case_sensitive=False), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/commands/calendar.py` at line 83, The CLI accepts any string for period while get_events() only handles "day", "week", and "month"; add validation for the period parameter (the typer.Option("month", "--period", ...)) at the start of the calendar command handler to ensure period is one of {"day","week","month"}, and if not raise a clear typer.BadParameter (or typer.Exit) with a helpful message; alternatively replace the Option with a click.Choice-backed Option so Typer enforces the choices, but either approach must reference the same period value used when calling get_events().pyicloud/cli/commands/hidemyemail.py (1)
39-50: Extract shared command bootstrap to reduce repetition.The same option/state/api bootstrap block is repeated in all seven commands. A small helper would reduce drift and make future option changes safer.
♻️ Suggested refactor sketch
+def _resolve_state_and_api( + ctx: typer.Context, + *, + username: UsernameOption, + session_dir: SessionDirOption, + http_proxy: HttpProxyOption, + https_proxy: HttpsProxyOption, + no_verify_ssl: NoVerifySslOption, + output_format: OutputFormatOption, + log_level: LogLevelOption, +): + store_command_options( + ctx, + username=username, + session_dir=session_dir, + http_proxy=http_proxy, + https_proxy=https_proxy, + no_verify_ssl=no_verify_ssl, + output_format=output_format, + log_level=log_level, + ) + state = get_state(ctx) + return state, state.get_api()- store_command_options(...) - state = get_state(ctx) - api = state.get_api() + state, api = _resolve_state_and_api( + ctx, + username=username, + session_dir=session_dir, + http_proxy=http_proxy, + https_proxy=https_proxy, + no_verify_ssl=no_verify_ssl, + output_format=output_format, + log_level=log_level, + )Also applies to: 84-95, 124-135, 163-174, 200-211, 237-248, 274-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/commands/hidemyemail.py` around lines 39 - 50, Extract the repeated bootstrap block into a small helper function (e.g., ensure_api(ctx, username=None, session_dir=None, http_proxy=None, https_proxy=None, no_verify_ssl=None, output_format=None, log_level=None)) that calls store_command_options(ctx, ...) and then returns state and api by calling get_state(ctx) and state.get_api(); replace the duplicated sequences in the seven command functions with a single call to this helper and use its returned state/api, ensuring the helper name (ensure_api) and the originals store_command_options, get_state, and state.get_api are referenced so reviewers can find the replaced code.pyicloud/cli/account_index.py (1)
73-98: Lock file cleanup has a minor race window, but atomic saves provide data integrity.The lock file is deleted after releasing the lock (lines 92-97), creating a small window where:
- Process A releases lock
- Process B acquires lock on the same file
- Process A unlinks the file
- Process C creates a new file at the same path and acquires a lock
Both B and C could proceed concurrently. However, since
_save_accounts_to_pathusestempfile+os.replacefor atomic writes, the actual data integrity is preserved. The worst outcome is redundant work, not corruption.Consider either:
- Keeping the lock file around (don't delete it)
- Or accepting this minor race since atomic saves provide the real guarantee
💡 Optional: Keep the lock file to eliminate the race
fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) try: yield index_path finally: fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) - try: - lock_path.unlink() - except FileNotFoundError: - pass - except OSError: - pass + # Lock file is intentionally kept to avoid race conditions + # during concurrent CLI invocations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/account_index.py` around lines 73 - 98, The unlock cleanup currently unlinks the lock file after releasing the fcntl lock in _locked_index (using account_index_path) which creates a small race; to fix, either remove the unlink logic entirely (leave the lock file in place) by deleting the try/except block that calls lock_path.unlink() and its OSError/FileNotFoundError handlers, or explicitly document and accept the minor race because _save_accounts_to_path already guarantees atomic writes via tempfile + os.replace — update _locked_index accordingly and add a brief comment referencing _save_accounts_to_path to justify keeping the unlink omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyicloud/cli/commands/devices.py`:
- Around line 341-393: The Typer option for the parameter raw in devices_export
is using "--raw" but should use the open/closed flag syntax to support a
tri-state (None/True/False); update the typer.Option call for the raw parameter
in the devices_export function to use "--raw/--no-raw" (keeping the default None
and the same help text) so Typer correctly distinguishes unset, true, and false
values.
In `@pyicloud/cli/commands/hidemyemail.py`:
- Around line 150-152: The update command currently sets note="Generated" by
default which overwrites existing notes; change the note parameter to be
optional (e.g., note: Optional[str] = typer.Option(None, "--note", help="Alias
note.")) in the hidemyemail_update handler and in the other similar command at
lines ~175-177, and when constructing the update payload only include the note
field if note is not None (or fetch and preserve the existing note before
updating); reference the function name hidemyemail_update and the parameters
anonymous_id, label, note to locate and fix the behavior.
- Around line 175-183: The success print in the Hide My Email mutator (where
payload = service_call(... api.hidemyemail.update_metadata ...),
state.write_json, and state.console.print are used) occurs without verifying the
API returned the expected data; change the logic to validate payload contains
the expected key (e.g., check payload.get("anonymousId") or
payload.get("result", {}).get("anonymousId") depending on how response is
shaped) before printing "Updated ...". If the key is missing, raise an error
(RuntimeError) or call state.console.print with an explicit error/warning and
include the returned payload for debugging instead of printing success; apply
the same pattern to the other mutator blocks referenced (lines 212–220, 249–257,
286–294).
---
Nitpick comments:
In `@pyicloud/cli/account_index.py`:
- Around line 73-98: The unlock cleanup currently unlinks the lock file after
releasing the fcntl lock in _locked_index (using account_index_path) which
creates a small race; to fix, either remove the unlink logic entirely (leave the
lock file in place) by deleting the try/except block that calls
lock_path.unlink() and its OSError/FileNotFoundError handlers, or explicitly
document and accept the minor race because _save_accounts_to_path already
guarantees atomic writes via tempfile + os.replace — update _locked_index
accordingly and add a brief comment referencing _save_accounts_to_path to
justify keeping the unlink omitted.
In `@pyicloud/cli/commands/calendar.py`:
- Line 83: The CLI accepts any string for period while get_events() only handles
"day", "week", and "month"; add validation for the period parameter (the
typer.Option("month", "--period", ...)) at the start of the calendar command
handler to ensure period is one of {"day","week","month"}, and if not raise a
clear typer.BadParameter (or typer.Exit) with a helpful message; alternatively
replace the Option with a click.Choice-backed Option so Typer enforces the
choices, but either approach must reference the same period value used when
calling get_events().
In `@pyicloud/cli/commands/hidemyemail.py`:
- Around line 39-50: Extract the repeated bootstrap block into a small helper
function (e.g., ensure_api(ctx, username=None, session_dir=None,
http_proxy=None, https_proxy=None, no_verify_ssl=None, output_format=None,
log_level=None)) that calls store_command_options(ctx, ...) and then returns
state and api by calling get_state(ctx) and state.get_api(); replace the
duplicated sequences in the seven command functions with a single call to this
helper and use its returned state/api, ensuring the helper name (ensure_api) and
the originals store_command_options, get_state, and state.get_api are referenced
so reviewers can find the replaced code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0f2b78a-6f79-46ca-9618-d2ac1468e45e
📒 Files selected for processing (5)
pyicloud/cli/account_index.pypyicloud/cli/commands/calendar.pypyicloud/cli/commands/devices.pypyicloud/cli/commands/hidemyemail.pytests/test_cmdline.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pyicloud/cli/commands/devices.py (1)
382-388: Consider validating mutually exclusive options earlier.The conflict check between
--rawand--normalizedoccurs after authentication and device resolution have already completed. Moving this validation beforeget_state(ctx)orresolve_device()would provide faster feedback to users who supply invalid option combinations.This is a minor optimisation and the current behaviour is functionally correct.
♻️ Suggested improvement
def devices_export( ctx: typer.Context, ... ) -> None: """Export a device snapshot to JSON.""" + if raw and normalized: + raise typer.BadParameter("Choose either --raw or --normalized, not both.") + store_command_options( ctx, ... ) state = get_state(ctx) api = state.get_api() idevice = resolve_device(api, device) - if raw and normalized: - raise typer.BadParameter("Choose either --raw or --normalized, not both.") use_raw = raw is True and not normalized🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/commands/devices.py` around lines 382 - 388, Move the mutually-exclusive option check for raw vs normalized up so it runs before any expensive work: perform the "if raw and normalized: raise typer.BadParameter(...)" validation at the start of the command handler, before calling get_state(ctx) or resolve_device(), and then compute use_raw and payload after those validations; update the command function in devices.py (the handler that calls get_state and resolve_device) to validate options first so users get immediate feedback without authenticating/resolving devices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyicloud/cli/commands/hidemyemail.py`:
- Around line 107-116: The current flow treats a None alias from
api.hidemyemail.generate() as success; change it to abort when generation
returns no alias: after calling service_call("Hide My Email", lambda:
api.hidemyemail.generate(), account_name=api.account_name) check if alias is
falsy/None and, if so, produce an error message (via state.console.print or
state.write_json depending on state.json_output) and exit with a non-zero status
(or raise an appropriate exception) instead of writing {"email": null} or
printing an empty line; update the handling around payload/
state.write_json/state.console.print to only run when alias is present.
- Around line 147-156: The code prints a success token even when the reserve
response lacks anonymousId; update the post-service_call handling in the
hidemyemail.reserve flow to validate payload for the expected "anonymousId"
(and/or other required keys) before reporting success: if state.json_output call
state.write_json(payload) as before, otherwise check payload is a dict and
contains a non-empty "anonymousId" returned by api.hidemyemail.reserve; on
missing/invalid anonymousId, print a clear error via
state.console.print/state.console.error and return a non-zero exit (or raise an
exception) instead of printing "reserved". Use the existing symbols
service_call, api.hidemyemail.reserve, payload, state.json_output,
state.write_json, and state.console.print to locate and implement the change.
---
Nitpick comments:
In `@pyicloud/cli/commands/devices.py`:
- Around line 382-388: Move the mutually-exclusive option check for raw vs
normalized up so it runs before any expensive work: perform the "if raw and
normalized: raise typer.BadParameter(...)" validation at the start of the
command handler, before calling get_state(ctx) or resolve_device(), and then
compute use_raw and payload after those validations; update the command function
in devices.py (the handler that calls get_state and resolve_device) to validate
options first so users get immediate feedback without authenticating/resolving
devices.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 219efbc5-c81a-4fee-ba10-124d987f368e
📒 Files selected for processing (3)
pyicloud/cli/commands/devices.pypyicloud/cli/commands/hidemyemail.pytests/test_cmdline.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_cmdline.py (2)
860-864: Relax thebasicConfigassertion to avoid brittle failures.At Line 863, asserting the exact call signature can fail on harmless extra kwargs while behaviour stays correct. Prefer asserting that
levelis warning.✅ More resilient assertion
- basic_config.assert_called_once_with(level=context_module.logging.WARNING) + basic_config.assert_called_once() + assert basic_config.call_args.kwargs.get("level") == context_module.logging.WARNING🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cmdline.py` around lines 860 - 864, The test currently asserts the exact call signature of the patched basicConfig which is brittle; change the assertion to first ensure basic_config was called once (basic_config.assert_called_once()) and then inspect its call arguments to assert that the 'level' kwarg equals context_module.logging.WARNING (e.g., args, kwargs = basic_config.call_args; assert kwargs.get("level") == context_module.logging.WARNING), leaving other kwargs unconstrained; locate the call in tests/test_cmdline.py around the _invoke(FakeAPI(), "account", "summary") block where basic_config is patched.
398-500: Deduplicate the patch harness used by_invokehelpers.Lines 451-499 duplicate the same patch stack in
_invokeand_invoke_with_cli_args. Extracting this into a shared context manager will reduce drift and make future test updates safer.♻️ Refactor sketch
-from contextlib import nullcontext +from contextlib import contextmanager, nullcontext @@ +@contextmanager +def _patched_cli_dependencies( + fake_api: FakeAPI, keyring_passwords: Optional[set[str]] = None +): + with ( + patch.object(context_module, "PyiCloudService", return_value=fake_api), + patch.object( + context_module, "configurable_ssl_verification", return_value=nullcontext() + ), + patch.object(context_module, "confirm", return_value=False), + patch.object( + context_module.utils, + "password_exists_in_keyring", + side_effect=lambda candidate: candidate in (keyring_passwords or set()), + ), + patch.object( + context_module.utils, + "get_password_from_keyring", + side_effect=lambda candidate: ( + "stored-secret" if candidate in (keyring_passwords or set()) else None + ), + ), + ): + yield @@ - with ( - patch.object(context_module, "PyiCloudService", return_value=fake_api), - ... - ): + with _patched_cli_dependencies(fake_api, keyring_passwords): return runner.invoke(app, cli_args) @@ - with ( - patch.object(context_module, "PyiCloudService", return_value=fake_api), - ... - ): + with _patched_cli_dependencies(fake_api, keyring_passwords): return runner.invoke(app, cli_args)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cmdline.py` around lines 398 - 500, The two helpers _invoke and _invoke_with_cli_args duplicate the same patch stack; extract that stack into a shared context manager function (e.g., _patched_cli_context or cli_patches) that accepts a keyring_passwords parameter and yields the patched context, and replace the duplicated with(...) blocks in both _invoke and _invoke_with_cli_args with a single "with cli_patches(keyring_passwords):" use; the shared context must patch context_module.PyiCloudService (return_value=fake_api for _invoke or kept flexible), context_module.configurable_ssl_verification (return_value=nullcontext()), context_module.confirm (return_value=False), context_module.utils.password_exists_in_keyring (side_effect lambda candidate: candidate in (keyring_passwords or set())), and context_module.utils.get_password_from_keyring (side_effect lambda candidate: "stored-secret" if candidate in (keyring_passwords or set()) else None).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_cmdline.py`:
- Line 25: TEST_ROOT is a deterministic shared path causing test collisions;
change its creation to use a per-run temp directory (e.g., use
tempfile.mkdtemp() or tempfile.TemporaryDirectory and/or append a uuid4
timestamp) so each test run gets a unique TEST_ROOT. Update the TEST_ROOT
assignment in tests/test_cmdline.py (the TEST_ROOT variable) to derive from a
per-run temp directory and ensure any teardown/cleanup logic uses that same
value.
---
Nitpick comments:
In `@tests/test_cmdline.py`:
- Around line 860-864: The test currently asserts the exact call signature of
the patched basicConfig which is brittle; change the assertion to first ensure
basic_config was called once (basic_config.assert_called_once()) and then
inspect its call arguments to assert that the 'level' kwarg equals
context_module.logging.WARNING (e.g., args, kwargs = basic_config.call_args;
assert kwargs.get("level") == context_module.logging.WARNING), leaving other
kwargs unconstrained; locate the call in tests/test_cmdline.py around the
_invoke(FakeAPI(), "account", "summary") block where basic_config is patched.
- Around line 398-500: The two helpers _invoke and _invoke_with_cli_args
duplicate the same patch stack; extract that stack into a shared context manager
function (e.g., _patched_cli_context or cli_patches) that accepts a
keyring_passwords parameter and yields the patched context, and replace the
duplicated with(...) blocks in both _invoke and _invoke_with_cli_args with a
single "with cli_patches(keyring_passwords):" use; the shared context must patch
context_module.PyiCloudService (return_value=fake_api for _invoke or kept
flexible), context_module.configurable_ssl_verification
(return_value=nullcontext()), context_module.confirm (return_value=False),
context_module.utils.password_exists_in_keyring (side_effect lambda candidate:
candidate in (keyring_passwords or set())), and
context_module.utils.get_password_from_keyring (side_effect lambda candidate:
"stored-secret" if candidate in (keyring_passwords or set()) else None).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f380041-f712-4fd1-bbcb-1126049c07d8
📒 Files selected for processing (2)
pyicloud/cli/commands/hidemyemail.pytests/test_cmdline.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyicloud/cli/commands/hidemyemail.py
Owner
|
@MrJarnould - This is really good. I like this a lot. Thanks for all your hard work 🙏 |
timlaing
added a commit
to MrJarnould/pyicloud
that referenced
this pull request
Mar 22, 2026
…dling (timlaing#203) * Refactor CLI into Typer command package * Finish Typer CLI contract cleanup * Add explicit auth and session CLI * Support final-command CLI options * Polish CLI help and shared messages * Make CLI options leaf-only * Refine auth status text output * Add subtle color to CLI tables * Demote internal warning logs * Trim CLI extraction to upstream scope * Fix README example correctness * Fix CLI session reauth and test stability * Stabilize CLI surface and address review feedback * Tighten CLI review follow-ups * Fix export and alias update edge cases * Handle empty Hide My Email responses * Isolate CLI test temp roots * Addressed review comments. --------- Co-authored-by: Tim Laing <11019084+timlaing@users.noreply.github.com>
timlaing
added a commit
to MrJarnould/pyicloud
that referenced
this pull request
Mar 22, 2026
…dling (timlaing#203) * Refactor CLI into Typer command package * Finish Typer CLI contract cleanup * Add explicit auth and session CLI * Support final-command CLI options * Polish CLI help and shared messages * Make CLI options leaf-only * Refine auth status text output * Add subtle color to CLI tables * Demote internal warning logs * Trim CLI extraction to upstream scope * Fix README example correctness * Fix CLI session reauth and test stability * Stabilize CLI surface and address review feedback * Tighten CLI review follow-ups * Fix export and alias update edge cases * Handle empty Hide My Email responses * Isolate CLI test temp roots * Addressed review comments. --------- Co-authored-by: Tim Laing <11019084+timlaing@users.noreply.github.com>
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
2 participants
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.
Summary
This PR refactors the
icloudCLI into a Typer-based command package while keeping the existingicloudentry point intact.The new structure groups functionality by service and authentication workflow, which makes the CLI easier to discover, extend, and test. It also tightens session handling for persisted sessions so service commands fail with actionable CLI messages instead of raw tracebacks when re-authentication is required.
What changed
icloudentry point throughpyicloud.cmdlineicloud auth loginicloud auth statusicloud auth logoutaccountdevicescalendarcontactsdrivephotoshidemyemailauth loginCompatibility
icloudcommand remains the main entry pointValidation
I validated this change with:
ruff check pyicloud/cli pyicloud/base.py pyicloud/session.py tests/test_cmdline.py tests/test_output.pypython -m pytest tests/test_cmdline.py tests/test_base.py tests/test_output.py -qResult:
140 passedI also ran manual smoke tests against a real account for:
icloud auth loginicloud auth statusicloud account summaryicloud account devicesicloud account storageicloud devices listicloud devices showicloud devices show --raw --format jsonicloud devices exporticloud auth logoutThese completed successfully and post-logout service commands returned the expected "not logged in" CLI message.