From 8878e92073c7be82d2144cf8f070807cec3a6b3a Mon Sep 17 00:00:00 2001 From: rafsanneloy Date: Sat, 11 Jul 2026 02:45:24 +0600 Subject: [PATCH 1/3] implementation phase_2 items Signed-off-by: rafsanneloy --- cbrain_cli/cli_utils.py | 43 ++++++++++++------- cbrain_cli/config.py | 6 +++ cbrain_cli/data/files.py | 6 +-- cbrain_cli/data/projects.py | 4 +- cbrain_cli/data/tools.py | 4 +- .../formatter/background_activities_fmt.py | 6 +-- cbrain_cli/formatter/files_fmt.py | 16 ++++++- cbrain_cli/formatter/tags_fmt.py | 26 ++++++++++- cbrain_cli/formatter/tasks_fmt.py | 4 +- cbrain_cli/formatter/tool_configs_fmt.py | 2 + cbrain_cli/handlers.py | 24 ++++++++--- cbrain_cli/sessions.py | 6 ++- cbrain_cli/users.py | 14 +++--- tests/conftest.py | 2 +- tests/test_cli_utils_output.py | 14 +++++- tests/test_exit_codes.py | 12 ++++++ tests/test_files.py | 4 +- tests/test_formatters.py | 8 ++++ tests/test_handlers.py | 4 +- tests/test_handlers_ops.py | 2 +- tests/test_sessions.py | 22 +++++++++- tests/test_tools.py | 2 +- 22 files changed, 177 insertions(+), 54 deletions(-) diff --git a/cbrain_cli/cli_utils.py b/cbrain_cli/cli_utils.py index 15191a7..e5b60d5 100644 --- a/cbrain_cli/cli_utils.py +++ b/cbrain_cli/cli_utils.py @@ -1,12 +1,13 @@ import functools +import importlib.metadata import json import re +import socket import urllib.error import urllib.parse import urllib.request -# import importlib.metadata -from cbrain_cli.config import DEFAULT_HEADERS, auth_headers, load_credentials +from cbrain_cli.config import DEFAULT_HEADERS, DEFAULT_TIMEOUT, auth_headers, load_credentials credentials = load_credentials() or {} cbrain_url = credentials.get("cbrain_url") @@ -136,11 +137,11 @@ def handle_connection_error(error): error_data = json.loads(error_response) if isinstance(error_data, dict): # Look for common error message fields - error_msg = ( + error_msg = str( error_data.get("message") or error_data.get("error") or error_data.get("notice") - or str(error_data) + or error_data ) # Check if this looks like a password change redirect if "change_password" in error_msg: @@ -186,7 +187,12 @@ def handle_connection_error(error): else: print(f"{status_description}: {error.reason}") elif isinstance(error, urllib.error.URLError): - if "Connection refused" in str(error): + if isinstance(error.reason, socket.timeout): + print( + f"Error: Request timed out after {DEFAULT_TIMEOUT}s. " + "Check your connection or set CBRAIN_TIMEOUT env var." + ) + elif "Connection refused" in str(error): print(f"Error: Cannot connect to CBRAIN server at {cbrain_url}") print("Please check if the CBRAIN server is running and accessible.") else: @@ -215,6 +221,10 @@ def wrapper(*args, **kwargs): except urllib.error.URLError as e: handle_connection_error(e) return 1 + except socket.timeout as e: + # Read-stage timeouts are raised bare, not wrapped in URLError. + handle_connection_error(urllib.error.URLError(e)) + return 1 except json.JSONDecodeError: print("Failed: Invalid response from server") return 1 @@ -245,14 +255,15 @@ def version_info(args): int Exit code (0 for success, 1 for failure) """ - print("cbrain cli client version 1.0") - # try: - # cbrain_cli_version = importlib.metadata.version('cbrain-cli') - # print(f"cbrain cli client version {cbrain_cli_version}") - # return 0 - # except importlib.metadata.PackageNotFoundError: - # print("Warning: Could not determine version. Package may not be installed properly.") - # return 1 + try: + cbrain_cli_version = importlib.metadata.version("cbrain-cli") + if output_json(args, {"version": cbrain_cli_version}): + return 0 + print(f"cbrain cli client version {cbrain_cli_version}") + return 0 + except importlib.metadata.PackageNotFoundError: + print("Warning: Could not determine version. Package may not be installed properly.") + return 1 def api_get(url, token, params=None): @@ -262,7 +273,7 @@ def api_get(url, token, params=None): if params: url = f"{url}?{urllib.parse.urlencode(params)}" req = urllib.request.Request(url, headers=auth_headers(token), method="GET") - with urllib.request.urlopen(req) as r: + with urllib.request.urlopen(req, timeout=DEFAULT_TIMEOUT) as r: return json.loads(r.read().decode()) @@ -273,7 +284,7 @@ def api_post_form(url, form_data, headers=None): headers = headers or DEFAULT_HEADERS body = urllib.parse.urlencode(form_data).encode() req = urllib.request.Request(url, data=body, headers=headers, method="POST") - with urllib.request.urlopen(req) as r: + with urllib.request.urlopen(req, timeout=DEFAULT_TIMEOUT) as r: return json.loads(r.read().decode()) @@ -287,7 +298,7 @@ def api_send(url, token, method="POST", payload=None): headers["Content-Type"] = "application/json" body = json.dumps(payload).encode() req = urllib.request.Request(url, data=body, headers=headers, method=method) - with urllib.request.urlopen(req) as r: + with urllib.request.urlopen(req, timeout=DEFAULT_TIMEOUT) as r: raw = r.read().decode() return (json.loads(raw) if raw.strip() else {}), r.status diff --git a/cbrain_cli/config.py b/cbrain_cli/config.py index 1f5894f..17af962 100644 --- a/cbrain_cli/config.py +++ b/cbrain_cli/config.py @@ -15,6 +15,12 @@ CREDENTIALS_FILE = SESSION_FILE_DIR / SESSION_FILE_NAME DEFAULT_CREDENTIALS_MODE = 0o600 +# HTTP request timeout in seconds; override with CBRAIN_TIMEOUT env var. +try: + DEFAULT_TIMEOUT = max(1, int(os.environ.get("CBRAIN_TIMEOUT", "30"))) +except ValueError: + DEFAULT_TIMEOUT = 30 + # HTTP headers. DEFAULT_HEADERS = { "Content-Type": "application/x-www-form-urlencoded", diff --git a/cbrain_cli/data/files.py b/cbrain_cli/data/files.py index b9fc20a..dcd48d4 100644 --- a/cbrain_cli/data/files.py +++ b/cbrain_cli/data/files.py @@ -11,7 +11,7 @@ cbrain_url, pagination, ) -from cbrain_cli.config import auth_headers +from cbrain_cli.config import DEFAULT_TIMEOUT, auth_headers def show_file(args): @@ -50,7 +50,7 @@ def upload_file(args): (response_data, response_status, file_name, file_size) or None if error """ # Check if file exists. - if not os.path.exists(args.file_path): + if not os.path.isfile(args.file_path): raise CliValidationError(f"File not found: {args.file_path}", field="file_path") if args.group_id is None: @@ -92,7 +92,7 @@ def upload_file(args): request = urllib.request.Request( f"{cbrain_url}/userfiles", data=body, headers=headers, method="POST" ) - with urllib.request.urlopen(request) as response: + with urllib.request.urlopen(request, timeout=DEFAULT_TIMEOUT) as response: response_data = json.loads(response.read().decode("utf-8")) return response_data, response.status, file_name, file_size, args.data_provider diff --git a/cbrain_cli/data/projects.py b/cbrain_cli/data/projects.py index c399988..15e45b9 100644 --- a/cbrain_cli/data/projects.py +++ b/cbrain_cli/data/projects.py @@ -42,7 +42,9 @@ def switch_project(args): f"Invalid group ID '{group_id}'. Must be a number or 'all'", field="group_id" ) from None - api_send(f"{cbrain_url}/groups/switch?id={group_id}", api_token) + _, switch_status = api_send(f"{cbrain_url}/groups/switch?id={group_id}", api_token) + if switch_status not in (200, 201, 204): + raise CliApiError(f"Failed to switch project (HTTP {switch_status})") group_data = api_get(f"{cbrain_url}/groups/{group_id}", api_token) credentials = load_credentials() diff --git a/cbrain_cli/data/tools.py b/cbrain_cli/data/tools.py index bd86b93..cd93751 100644 --- a/cbrain_cli/data/tools.py +++ b/cbrain_cli/data/tools.py @@ -35,10 +35,10 @@ def show_tool(args): api_token, {"page": str(page), "per_page": str(per_page)}, ) - if not tools_page: + if not tools_page or not isinstance(tools_page, list): break for tool in tools_page: - if tool.get("id") == tool_id: + if str(tool.get("id")) == str(tool_id): return tool if len(tools_page) < per_page: break diff --git a/cbrain_cli/formatter/background_activities_fmt.py b/cbrain_cli/formatter/background_activities_fmt.py index 5245198..62be123 100644 --- a/cbrain_cli/formatter/background_activities_fmt.py +++ b/cbrain_cli/formatter/background_activities_fmt.py @@ -29,11 +29,7 @@ def print_activities_list(activities_data, args): "remote_resource_id": a.get("remote_resource_id", ""), "status": a.get("status", ""), "created_at": ( - a.get("created_at", "").split("T")[0] - + " " - + a.get("created_at", "").split("T")[1].split(".")[0] - if a.get("created_at") - else "" + a["created_at"].replace("T", " ").split(".")[0] if a.get("created_at") else "" ), "items": ",".join(map(str, a.get("items", []))) if a.get("items") else "", "num_successes": a.get("num_successes", 0), diff --git a/cbrain_cli/formatter/files_fmt.py b/cbrain_cli/formatter/files_fmt.py index e2ee24d..f20efea 100644 --- a/cbrain_cli/formatter/files_fmt.py +++ b/cbrain_cli/formatter/files_fmt.py @@ -58,7 +58,9 @@ def print_files_list(files_data, args): dynamic_table_print(files_data, ["id", "type", "name"], ["ID", "Type", "File Name"]) -def print_upload_result(response_data, response_status, file_name, file_size, data_provider_id): +def print_upload_result( + response_data, response_status, file_name, file_size, data_provider_id, args=None +): """ Print the result of a file upload operation. @@ -74,7 +76,12 @@ def print_upload_result(response_data, response_status, file_name, file_size, da Size of the uploaded file in bytes data_provider_id : int ID of the data provider + args : argparse.Namespace, optional + Command line arguments, including the --json flag """ + if args is not None and output_json(args, response_data): + return + print(f"Uploading {file_name} ({file_size} bytes) to data provider {data_provider_id}...") if response_status == 200 or response_status == 201: @@ -83,7 +90,7 @@ def print_upload_result(response_data, response_status, file_name, file_size, da print(f"Server response: {response_data['notice']}") -def print_move_copy_result(response_data, response_status, operation="move"): +def print_move_copy_result(response_data, response_status, operation="move", args=None): """ Print the result of a file move or copy operation. @@ -95,7 +102,12 @@ def print_move_copy_result(response_data, response_status, operation="move"): HTTP status code operation : str Operation type ("move" or "copy") + args : argparse.Namespace, optional + Command line arguments, including the --json flag """ + if args is not None and output_json(args, response_data): + return + if response_status == 200 or response_status == 201: # Show the message from the API response message = response_data.get("message", "").strip() diff --git a/cbrain_cli/formatter/tags_fmt.py b/cbrain_cli/formatter/tags_fmt.py index 436cebb..2483f56 100644 --- a/cbrain_cli/formatter/tags_fmt.py +++ b/cbrain_cli/formatter/tags_fmt.py @@ -58,7 +58,13 @@ def print_tag_details(tag_data, args): def print_tag_operation_result( - operation, tag_id=None, success=True, error_msg=None, response_status=None + operation, + tag_id=None, + success=True, + error_msg=None, + response_status=None, + args=None, + response_data=None, ): """ Print result of a tag operation (create, update, delete). @@ -75,7 +81,25 @@ def print_tag_operation_result( Error message if operation failed response_status : int, optional HTTP response status code if operation failed + args : argparse.Namespace, optional + Command line arguments, including the --json flag + response_data : dict, optional + Raw API response data for structured output """ + if args is not None: + structured = ( + response_data + if response_data + else { + "operation": operation, + "success": success, + "tag_id": tag_id, + "error": error_msg, + } + ) + if output_json(args, structured): + return + if success: if operation == "create": print("Tag created successfully!") diff --git a/cbrain_cli/formatter/tasks_fmt.py b/cbrain_cli/formatter/tasks_fmt.py index 55edb09..972606b 100644 --- a/cbrain_cli/formatter/tasks_fmt.py +++ b/cbrain_cli/formatter/tasks_fmt.py @@ -27,7 +27,7 @@ def print_task_data(tasks_data, args): formatted_tasks = [ { "id": task.get("id", ""), - "type": task.get("type", "").replace("BoutiquesTask::", ""), + "type": (task.get("type") or "").replace("BoutiquesTask::", ""), "status": task.get("status", ""), "bourreau_id": task.get("bourreau_id", ""), "user_id": task.get("user_id", ""), @@ -108,7 +108,7 @@ def print_task_details(task_data, args): print() print("DESCRIPTION") print("-" * 30) - for line in task_data.get("description").strip().split("\n"): + for line in str(task_data.get("description", "")).strip().split("\n"): print(line) # Display params if they exist. diff --git a/cbrain_cli/formatter/tool_configs_fmt.py b/cbrain_cli/formatter/tool_configs_fmt.py index c8c58e7..e2ddc15 100644 --- a/cbrain_cli/formatter/tool_configs_fmt.py +++ b/cbrain_cli/formatter/tool_configs_fmt.py @@ -88,4 +88,6 @@ def print_boutiques_descriptor(boutiques_descriptor, args): print("No Boutiques descriptor found.") return + if output_json(args, boutiques_descriptor): + return json_printer(boutiques_descriptor) diff --git a/cbrain_cli/handlers.py b/cbrain_cli/handlers.py index 578e413..61bf2b4 100644 --- a/cbrain_cli/handlers.py +++ b/cbrain_cli/handlers.py @@ -5,7 +5,7 @@ and format their output appropriately. """ -from cbrain_cli.cli_utils import json_printer +from cbrain_cli.cli_utils import json_printer, output_json from cbrain_cli.data import ( background_activities, data_providers, @@ -56,7 +56,7 @@ def handle_file_upload(args): result = files.upload_file(args) if result is None: return 1 - files_fmt.print_upload_result(*result) + files_fmt.print_upload_result(*result, args=args) if result[1] not in (200, 201): return 1 @@ -66,7 +66,7 @@ def handle_file_copy(args): result = files.copy_file(args) if result is None: return 1 - files_fmt.print_move_copy_result(*result, operation="copy") + files_fmt.print_move_copy_result(*result, operation="copy", args=args) if result[1] not in (200, 201): return 1 @@ -76,7 +76,7 @@ def handle_file_move(args): result = files.move_file(args) if result is None: return 1 - files_fmt.print_move_copy_result(*result, operation="move") + files_fmt.print_move_copy_result(*result, operation="move", args=args) if result[1] not in (200, 201): return 1 @@ -111,6 +111,8 @@ def handle_dataprovider_is_alive(args): result = data_providers.is_alive(args) if result is None: return 1 + if output_json(args, result): + return json_printer(result) @@ -119,6 +121,8 @@ def handle_dataprovider_delete_unregistered(args): result = data_providers.delete_unregistered_files(args) if result is None: return 1 + if output_json(args, result): + return json_printer(result) @@ -161,6 +165,8 @@ def handle_project_show(args): def handle_project_unswitch(args): """Unswitch from current project context.""" result = projects.unswitch_project(args) + if result is None: + return 1 projects_fmt.print_unswitch_result(result, args) @@ -229,7 +235,12 @@ def handle_tag_create(args): if result is None: return 1 tags_fmt.print_tag_operation_result( - "create", success=result[1], error_msg=result[2], response_status=result[3] + "create", + success=result[1], + error_msg=result[2], + response_status=result[3], + args=args, + response_data=result[0], ) if not result[1]: return 1 @@ -246,6 +257,8 @@ def handle_tag_update(args): success=result[1], error_msg=result[2], response_status=result[3], + args=args, + response_data=result[0], ) if not result[1]: return 1 @@ -262,6 +275,7 @@ def handle_tag_delete(args): success=result[0], error_msg=result[1], response_status=result[2], + args=args, ) if not result[0]: return 1 diff --git a/cbrain_cli/sessions.py b/cbrain_cli/sessions.py index f5b66e5..9f00cdb 100644 --- a/cbrain_cli/sessions.py +++ b/cbrain_cli/sessions.py @@ -29,8 +29,10 @@ def create_session(args): """ if CREDENTIALS_FILE.exists(): - print("Already logged in. Use 'cbrain logout' to logout.") - return 1 + creds = load_credentials() + if creds and creds.get("api_token") and creds.get("cbrain_url"): + print("Already logged in. Use 'cbrain logout' to logout.") + return 1 # Get user input. cbrain_url = input("Enter CBRAIN server base URL [default: localhost:3000]: ").strip() diff --git a/cbrain_cli/users.py b/cbrain_cli/users.py index 65899d4..eae1bce 100644 --- a/cbrain_cli/users.py +++ b/cbrain_cli/users.py @@ -9,7 +9,7 @@ json_printer, user_id, ) -from cbrain_cli.config import auth_headers +from cbrain_cli.config import DEFAULT_TIMEOUT, auth_headers def user_details(user_id): @@ -33,7 +33,7 @@ def user_details(user_id): ) try: - with urllib.request.urlopen(user_request) as response: + with urllib.request.urlopen(user_request, timeout=DEFAULT_TIMEOUT) as response: user_data = json.loads(response.read().decode("utf-8")) return user_data @@ -79,8 +79,8 @@ def whoami_user(args): # Handle JSON output first if getattr(args, "json", False): output = { - "login": user_data["login"], - "full_name": user_data["full_name"], + "login": user_data.get("login", ""), + "full_name": user_data.get("full_name", ""), "server": cbrain_url, } json_printer(output) @@ -95,7 +95,7 @@ def whoami_user(args): ) try: - with urllib.request.urlopen(session_request) as response: + with urllib.request.urlopen(session_request, timeout=DEFAULT_TIMEOUT) as response: session_data = json.loads(response.read().decode("utf-8")) # Verify local credentials match server response. @@ -115,4 +115,6 @@ def whoami_user(args): print(f"Error verifying session: {e}") return 1 - print(f"Current user: {user_data['login']} ({user_data['full_name']}) on server {cbrain_url}") + login = user_data.get("login", "") + full_name = user_data.get("full_name", "") + print(f"Current user: {login} ({full_name}) on server {cbrain_url}") diff --git a/tests/conftest.py b/tests/conftest.py index 1e854bc..118e0ff 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -78,7 +78,7 @@ def capture_urlopen(monkeypatch): """ def configure(response_json=None, status=200, raw_body=None, side_effect=None): - def fake_urlopen(request): + def fake_urlopen(request, **kwargs): captured["url"] = request.full_url captured["headers"] = request.headers captured["data"] = request.data diff --git a/tests/test_cli_utils_output.py b/tests/test_cli_utils_output.py index 8bc51cc..391086e 100644 --- a/tests/test_cli_utils_output.py +++ b/tests/test_cli_utils_output.py @@ -54,5 +54,17 @@ def test_dynamic_table_print_header_mismatch_raises(): def test_version_info(capsys): - version_info(MagicMock()) + version_info(MagicMock(json=False, jsonl=False)) assert "cbrain cli client version" in capsys.readouterr().out + + +def test_version_info_json(capsys): + version_info(MagicMock(json=True, jsonl=False)) + out = json.loads(capsys.readouterr().out) + assert "version" in out + + +def test_version_info_jsonl(capsys): + version_info(MagicMock(json=False, jsonl=True)) + out = json.loads(capsys.readouterr().out.strip()) + assert "version" in out diff --git a/tests/test_exit_codes.py b/tests/test_exit_codes.py index b2fa765..21188b6 100644 --- a/tests/test_exit_codes.py +++ b/tests/test_exit_codes.py @@ -1,5 +1,6 @@ import io import json +import socket from unittest.mock import MagicMock from urllib.error import HTTPError, URLError @@ -130,6 +131,17 @@ def test_handle_connection_error_html_body(capsys): assert "Record missing" in out +def test_handle_connection_error_timeout(capsys): + handle_connection_error(URLError(socket.timeout("timed out"))) + assert "timed out" in capsys.readouterr().out + + +def test_handle_errors_bare_read_timeout(capsys): + result = handle_errors(MagicMock(side_effect=socket.timeout("timed out")))() + assert result == 1 + assert "timed out" in capsys.readouterr().out + + def test_handle_connection_error_generic_url_error(capsys, monkeypatch): monkeypatch.setattr("cbrain_cli.cli_utils.cbrain_url", URL) handle_connection_error(URLError("timed out")) diff --git a/tests/test_files.py b/tests/test_files.py index 00c4a99..56384e1 100644 --- a/tests/test_files.py +++ b/tests/test_files.py @@ -43,7 +43,7 @@ def test_list_files_empty_list_is_not_error(mock_urlopen): def test_list_files_passes_filter_params(monkeypatch): captured = {} - def fake_urlopen(req): + def fake_urlopen(req, **kwargs): from unittest.mock import MagicMock captured["url"] = req.full_url @@ -112,7 +112,7 @@ def test_upload_file_success(monkeypatch, tmp_path): upload_path.write_bytes(b"payload") captured = {} - def fake_urlopen(request): + def fake_urlopen(request, **kwargs): captured["content_type"] = request.headers.get("Content-type", "") from unittest.mock import MagicMock diff --git a/tests/test_formatters.py b/tests/test_formatters.py index c00a73a..8a70862 100644 --- a/tests/test_formatters.py +++ b/tests/test_formatters.py @@ -242,6 +242,14 @@ def test_print_tag_operation_result_success(capsys): assert "created successfully" in capsys.readouterr().out +def test_print_tag_operation_result_failure_json_includes_error(capsys): + tags_fmt.print_tag_operation_result( + "create", success=False, error_msg="name taken", args=make_args(json=True) + ) + result = parse_json_output(capsys) + assert result["error"] == "name taken" + + def test_print_projects_list_with_data(capsys): projects_fmt.print_projects_list([{"id": 1, "type": "Group", "name": "A"}], make_args()) assert "Group" in capsys.readouterr().out diff --git a/tests/test_handlers.py b/tests/test_handlers.py index dc31e73..bfc2fda 100644 --- a/tests/test_handlers.py +++ b/tests/test_handlers.py @@ -101,10 +101,10 @@ def test_handle_project_show_no_project_prints_message(monkeypatch, capsys): assert "No active project." in capsys.readouterr().out -def test_handle_project_unswitch_always_returns_none(monkeypatch): +def test_handle_project_unswitch_returns_1_on_none(monkeypatch): monkeypatch.setattr("cbrain_cli.handlers.projects.unswitch_project", lambda _: None) monkeypatch.setattr("cbrain_cli.handlers.projects_fmt.print_unswitch_result", lambda *_: None) - assert handle_project_unswitch(make_args()) is None + assert handle_project_unswitch(make_args()) == 1 def test_handle_project_switch_json_output(monkeypatch, capsys): diff --git a/tests/test_handlers_ops.py b/tests/test_handlers_ops.py index ee90d9d..9f4681b 100644 --- a/tests/test_handlers_ops.py +++ b/tests/test_handlers_ops.py @@ -136,7 +136,7 @@ def test_handle_project_switch_none_returns_1(monkeypatch): @pytest.mark.parametrize("upload_result,expected", FILE_UPLOAD_CASES) def test_handle_file_upload(monkeypatch, upload_result, expected): monkeypatch.setattr("cbrain_cli.handlers.files.upload_file", lambda _: upload_result) - monkeypatch.setattr("cbrain_cli.handlers.files_fmt.print_upload_result", lambda *_: None) + monkeypatch.setattr("cbrain_cli.handlers.files_fmt.print_upload_result", lambda *_, **__: None) result = handlers.handle_file_upload(make_args()) assert result is expected if expected is None else result == expected diff --git a/tests/test_sessions.py b/tests/test_sessions.py index d67ae99..6628392 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -9,12 +9,32 @@ def test_create_session_already_logged_in(sessions_creds_file, capsys): - sessions_creds_file.write_text("{}") + import json + + sessions_creds_file.write_text( + json.dumps({"api_token": "tok", "cbrain_url": "http://localhost:3000"}) + ) result = create_session(argparse.Namespace()) assert result == 1 assert "Already logged in" in capsys.readouterr().out +def test_create_session_empty_credentials_file_allows_login( + sessions_creds_file, monkeypatch, capsys +): + """Empty/corrupt credentials file should not block re-login.""" + sessions_creds_file.write_text("{}") + inputs = iter(["http://localhost:3000", "admin"]) + monkeypatch.setattr("builtins.input", lambda _: next(inputs)) + monkeypatch.setattr("getpass.getpass", lambda _: "secret") + monkeypatch.setattr( + "cbrain_cli.sessions.api_post_form", + lambda *_a, **_k: {"cbrain_api_token": "newtok", "user_id": 1}, + ) + result = create_session(argparse.Namespace()) + assert result == 0 + + def test_create_session_empty_username_raises(monkeypatch, sessions_creds_file): inputs = iter(["http://localhost:3000", ""]) monkeypatch.setattr("builtins.input", lambda _: next(inputs)) diff --git a/tests/test_tools.py b/tests/test_tools.py index 3bbcde3..62370c1 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -16,7 +16,7 @@ def _patch_tools_locals(monkeypatch): def test_list_tools_passes_page_and_per_page(monkeypatch): captured = {} - def fake_urlopen(request): + def fake_urlopen(request, **kwargs): captured["url"] = request.full_url mock_http_response = MagicMock() mock_http_response.__enter__.return_value.read.return_value = b"[]" From 04a68d7d296b21967daecc040415af2cc637fdc7 Mon Sep 17 00:00:00 2001 From: rafsanneloy Date: Tue, 14 Jul 2026 16:08:05 +0600 Subject: [PATCH 2/3] Implement confirmation prompts for destructive actions and update version handling Signed-off-by: rafsanneloy --- capture_tests/cbrain_cli_commands | 4 +- capture_tests/expected_captures.txt | 24 +++++---- cbrain_cli/cli_utils.py | 52 ++++++++++++++++--- cbrain_cli/formatter/tags_fmt.py | 21 +++----- cbrain_cli/handlers.py | 12 +++-- cbrain_cli/main.py | 9 ++++ tests/test_cli_utils_output.py | 78 ++++++++++++++++++++++++++++- tests/test_handlers_ops.py | 20 ++++++-- 8 files changed, 181 insertions(+), 39 deletions(-) diff --git a/capture_tests/cbrain_cli_commands b/capture_tests/cbrain_cli_commands index f709cbc..09154a1 100755 --- a/capture_tests/cbrain_cli_commands +++ b/capture_tests/cbrain_cli_commands @@ -96,7 +96,7 @@ cbrain --jsonl tag show 21 cbrain tag show 99 cbrain tag update 99 --name Renamed --user-id 2 --group-id 3 cbrain tag show 99 -cbrain tag delete 99 +cbrain tag delete 99 --yes cbrain tag create --name NewTag1 --user-id 2 --group-id 3 cbrain --json tag create --name NewTag2 --user-id 2 --group-id 3 @@ -165,7 +165,7 @@ cbrain file show 2 cbrain --json file show 2 cbrain --jsonl file show 2 -cbrain file delete 5 +cbrain file delete 5 --yes cbrain file move --file-id 2 --dp-id 15 #cbrain --json file move --file-id 2 --dp-id 15 diff --git a/capture_tests/expected_captures.txt b/capture_tests/expected_captures.txt index 3cb06d6..23ab5ee 100644 --- a/capture_tests/expected_captures.txt +++ b/capture_tests/expected_captures.txt @@ -16,22 +16,24 @@ Stderr: ############################ Command: cbrain version Status: 0 -Stdout: 30 bytes +Stdout: 32 bytes Stderr: 0 bytes Stdout: -cbrain cli client version 1.0 +cbrain cli client version 0.1.0 Stderr: (No output) ############################ Command: cbrain --json version Status: 0 -Stdout: 30 bytes +Stdout: 25 bytes Stderr: 0 bytes Stdout: -cbrain cli client version 1.0 +{ + "version": "0.1.0" +} Stderr: (No output) @@ -74,22 +76,24 @@ Stderr: ############################ Command: cbrain version Status: 0 -Stdout: 30 bytes +Stdout: 32 bytes Stderr: 0 bytes Stdout: -cbrain cli client version 1.0 +cbrain cli client version 0.1.0 Stderr: (No output) ############################ Command: cbrain --json version Status: 0 -Stdout: 30 bytes +Stdout: 25 bytes Stderr: 0 bytes Stdout: -cbrain cli client version 1.0 +{ + "version": "0.1.0" +} Stderr: (No output) @@ -744,7 +748,7 @@ Stderr: (No output) ############################ -Command: cbrain tag delete 99 +Command: cbrain tag delete 99 --yes Status: 0 Stdout: 29 bytes Stderr: 0 bytes @@ -1638,7 +1642,7 @@ Stderr: (No output) ############################ -Command: cbrain file delete 5 +Command: cbrain file delete 5 --yes Status: 0 Stdout: 71 bytes Stderr: 0 bytes diff --git a/cbrain_cli/cli_utils.py b/cbrain_cli/cli_utils.py index e5b60d5..c0edbe4 100644 --- a/cbrain_cli/cli_utils.py +++ b/cbrain_cli/cli_utils.py @@ -1,11 +1,14 @@ +import configparser import functools import importlib.metadata import json import re import socket +import sys import urllib.error import urllib.parse import urllib.request +from pathlib import Path from cbrain_cli.config import DEFAULT_HEADERS, DEFAULT_TIMEOUT, auth_headers, load_credentials @@ -245,6 +248,9 @@ def version_info(args): """ Display the CLI version information. + Prefer installed package metadata; fall back to setup.cfg when running + from a source tree without install. + Parameters ---------- args : argparse.Namespace @@ -257,13 +263,15 @@ def version_info(args): """ try: cbrain_cli_version = importlib.metadata.version("cbrain-cli") - if output_json(args, {"version": cbrain_cli_version}): - return 0 - print(f"cbrain cli client version {cbrain_cli_version}") - return 0 except importlib.metadata.PackageNotFoundError: - print("Warning: Could not determine version. Package may not be installed properly.") - return 1 + cfg = configparser.ConfigParser() + cfg.read(Path(__file__).resolve().parents[1] / "setup.cfg") + cbrain_cli_version = cfg["metadata"]["version"] + + if output_json(args, {"version": cbrain_cli_version}): + return 0 + print(f"cbrain cli client version {cbrain_cli_version}") + return 0 def api_get(url, token, params=None): @@ -316,6 +324,38 @@ def output_json(args, data): return False +def confirm_destructive(args, prompt): + """ + Gate destructive actions: ``--yes`` skips, TTY asks, otherwise refuse. + + Returns + ------- + bool + True to proceed, False if the user declined an interactive prompt. + """ + if getattr(args, "yes", False): + return True + # JSON/JSONL must not mix with a prompt; pipes/EOF also never auto-confirm. + if ( + getattr(args, "json", False) + or getattr(args, "jsonl", False) + or not sys.stdin.isatty() + ): + raise CliValidationError( + "Refusing destructive action without confirmation; pass --yes", + field="--yes", + ) + try: + answer = input(f"{prompt} [y/N]: ").strip().lower() + except EOFError: + print("Aborted.") + return False + if answer in ("y", "yes"): + return True + print("Aborted.") + return False + + def display_key_value_table(pairs): """ Print a (key-value) two-column Field/Value table from a list of (field, value) tuples. diff --git a/cbrain_cli/formatter/tags_fmt.py b/cbrain_cli/formatter/tags_fmt.py index 2483f56..11f26ec 100644 --- a/cbrain_cli/formatter/tags_fmt.py +++ b/cbrain_cli/formatter/tags_fmt.py @@ -64,7 +64,6 @@ def print_tag_operation_result( error_msg=None, response_status=None, args=None, - response_data=None, ): """ Print result of a tag operation (create, update, delete). @@ -83,20 +82,14 @@ def print_tag_operation_result( HTTP response status code if operation failed args : argparse.Namespace, optional Command line arguments, including the --json flag - response_data : dict, optional - Raw API response data for structured output """ - if args is not None: - structured = ( - response_data - if response_data - else { - "operation": operation, - "success": success, - "tag_id": tag_id, - "error": error_msg, - } - ) + if args is not None and not success: + structured = { + "operation": operation, + "success": success, + "tag_id": tag_id, + "error": error_msg, + } if output_json(args, structured): return diff --git a/cbrain_cli/handlers.py b/cbrain_cli/handlers.py index 61bf2b4..f31b348 100644 --- a/cbrain_cli/handlers.py +++ b/cbrain_cli/handlers.py @@ -5,7 +5,7 @@ and format their output appropriately. """ -from cbrain_cli.cli_utils import json_printer, output_json +from cbrain_cli.cli_utils import confirm_destructive, json_printer, output_json from cbrain_cli.data import ( background_activities, data_providers, @@ -83,6 +83,8 @@ def handle_file_move(args): def handle_file_delete(args): """Delete a specific file from CBRAIN and display the deletion status.""" + if not confirm_destructive(args, f"Delete file {args.file_id}?"): + return 1 result = files.delete_file(args) if result is None: return 1 @@ -118,6 +120,10 @@ def handle_dataprovider_is_alive(args): def handle_dataprovider_delete_unregistered(args): """Remove unregistered files from a data provider and display the cleanup results.""" + if not confirm_destructive( + args, f"Delete unregistered files on data provider {args.id}?" + ): + return 1 result = data_providers.delete_unregistered_files(args) if result is None: return 1 @@ -240,7 +246,6 @@ def handle_tag_create(args): error_msg=result[2], response_status=result[3], args=args, - response_data=result[0], ) if not result[1]: return 1 @@ -258,7 +263,6 @@ def handle_tag_update(args): error_msg=result[2], response_status=result[3], args=args, - response_data=result[0], ) if not result[1]: return 1 @@ -266,6 +270,8 @@ def handle_tag_update(args): def handle_tag_delete(args): """Delete a specific tag from CBRAIN and display the deletion result.""" + if not confirm_destructive(args, f"Delete tag {args.tag_id}?"): + return 1 result = tags.delete_tag(args) if result is None: return 1 diff --git a/cbrain_cli/main.py b/cbrain_cli/main.py index 499cfa4..0649055 100644 --- a/cbrain_cli/main.py +++ b/cbrain_cli/main.py @@ -157,6 +157,9 @@ def build_parser(): # file delete file_delete_parser = file_subparsers.add_parser("delete", help="Delete a file") file_delete_parser.add_argument("file_id", type=int, help="ID of the file to delete") + file_delete_parser.add_argument( + "-y", "--yes", action="store_true", help="Skip confirmation prompt" + ) file_delete_parser.set_defaults(func=handle_errors(handle_file_delete)) # Data provider commands @@ -202,6 +205,9 @@ def build_parser(): dataprovider_delete_unregistered_files_parser.add_argument( "id", type=int, help="Data provider ID" ) + dataprovider_delete_unregistered_files_parser.add_argument( + "-y", "--yes", action="store_true", help="Skip confirmation prompt" + ) dataprovider_delete_unregistered_files_parser.set_defaults( func=handle_errors(handle_dataprovider_delete_unregistered) ) @@ -333,6 +339,9 @@ def build_parser(): type=int, help="Tag ID to delete", ) + tag_delete_parser.add_argument( + "-y", "--yes", action="store_true", help="Skip confirmation prompt" + ) tag_delete_parser.set_defaults(func=handle_errors(handle_tag_delete)) # Background activity commands diff --git a/tests/test_cli_utils_output.py b/tests/test_cli_utils_output.py index 391086e..2a436dd 100644 --- a/tests/test_cli_utils_output.py +++ b/tests/test_cli_utils_output.py @@ -1,9 +1,13 @@ +import configparser import json +from pathlib import Path from unittest.mock import MagicMock import pytest from cbrain_cli.cli_utils import ( + CliValidationError, + confirm_destructive, display_key_value_table, dynamic_table_print, jsonl_printer, @@ -11,6 +15,12 @@ ) +def _setup_cfg_version(): + cfg = configparser.ConfigParser() + cfg.read(Path(__file__).resolve().parents[1] / "setup.cfg") + return cfg["metadata"]["version"] + + def test_jsonl_printer_list(capsys): jsonl_printer([{"a": 1}, {"b": 2}]) lines = capsys.readouterr().out.strip().splitlines() @@ -55,7 +65,24 @@ def test_dynamic_table_print_header_mismatch_raises(): def test_version_info(capsys): version_info(MagicMock(json=False, jsonl=False)) - assert "cbrain cli client version" in capsys.readouterr().out + assert f"cbrain cli client version {_setup_cfg_version()}" in capsys.readouterr().out + + +def test_version_info_prefers_package_metadata(monkeypatch, capsys): + monkeypatch.setattr("importlib.metadata.version", lambda _name: "9.9.9-installed") + assert version_info(MagicMock(json=False, jsonl=False)) == 0 + assert "cbrain cli client version 9.9.9-installed" in capsys.readouterr().out + + +def test_version_info_falls_back_to_setup_cfg(monkeypatch, capsys): + import importlib.metadata + + def boom(_name): + raise importlib.metadata.PackageNotFoundError("cbrain-cli") + + monkeypatch.setattr(importlib.metadata, "version", boom) + assert version_info(MagicMock(json=False, jsonl=False)) == 0 + assert f"cbrain cli client version {_setup_cfg_version()}" in capsys.readouterr().out def test_version_info_json(capsys): @@ -68,3 +95,52 @@ def test_version_info_jsonl(capsys): version_info(MagicMock(json=False, jsonl=True)) out = json.loads(capsys.readouterr().out.strip()) assert "version" in out + + +def test_confirm_destructive_yes_flag(): + assert confirm_destructive(MagicMock(yes=True), "Delete?") is True + + +def test_confirm_destructive_noninteractive_requires_yes(monkeypatch): + monkeypatch.setattr("sys.stdin.isatty", lambda: False) + with pytest.raises(CliValidationError, match="--yes"): + confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") + + +def test_confirm_destructive_json_requires_yes(monkeypatch): + monkeypatch.setattr("sys.stdin.isatty", lambda: True) + with pytest.raises(CliValidationError, match="--yes"): + confirm_destructive(MagicMock(yes=False, json=True, jsonl=False), "Delete?") + + +def test_confirm_destructive_tty_accepts_y(monkeypatch): + monkeypatch.setattr("sys.stdin.isatty", lambda: True) + monkeypatch.setattr("builtins.input", lambda _: "y") + assert ( + confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") + is True + ) + + +def test_confirm_destructive_tty_declines(monkeypatch, capsys): + monkeypatch.setattr("sys.stdin.isatty", lambda: True) + monkeypatch.setattr("builtins.input", lambda _: "n") + assert ( + confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") + is False + ) + assert "Aborted." in capsys.readouterr().out + + +def test_confirm_destructive_tty_eof(monkeypatch, capsys): + monkeypatch.setattr("sys.stdin.isatty", lambda: True) + + def boom(_): + raise EOFError + + monkeypatch.setattr("builtins.input", boom) + assert ( + confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") + is False + ) + assert "Aborted." in capsys.readouterr().out diff --git a/tests/test_handlers_ops.py b/tests/test_handlers_ops.py index 9f4681b..c47843c 100644 --- a/tests/test_handlers_ops.py +++ b/tests/test_handlers_ops.py @@ -89,7 +89,7 @@ "handle_tag_delete", "cbrain_cli.handlers.tags.delete_tag", "cbrain_cli.handlers.tags_fmt.print_tag_operation_result", - make_args(tag_id=1), + make_args(tag_id=1, yes=True), (True, None, 200), None, ), @@ -115,7 +115,7 @@ def test_handle_dataprovider_delete_unregistered_prints_json(monkeypatch, capsys "cbrain_cli.handlers.data_providers.delete_unregistered_files", lambda _: {"deleted": 2}, ) - handlers.handle_dataprovider_delete_unregistered(make_args(id=1)) + handlers.handle_dataprovider_delete_unregistered(make_args(id=1, yes=True)) assert '"deleted": 2' in capsys.readouterr().out @@ -166,7 +166,21 @@ def test_handle_file_delete_success(monkeypatch): lambda _: {"deleted": 1}, ) monkeypatch.setattr("cbrain_cli.handlers.files_fmt.print_delete_result", lambda *_: None) - assert handlers.handle_file_delete(make_args(file_id=1)) is None + assert handlers.handle_file_delete(make_args(file_id=1, yes=True)) is None + + +def test_handle_file_delete_aborts_without_yes(monkeypatch): + called = [] + monkeypatch.setattr( + "cbrain_cli.handlers.files.delete_file", + lambda _: called.append(True) or {"deleted": 1}, + ) + monkeypatch.setattr( + "cbrain_cli.handlers.confirm_destructive", + lambda *_: False, + ) + assert handlers.handle_file_delete(make_args(file_id=1)) == 1 + assert called == [] def test_handle_tool_config_boutiques_descriptor(monkeypatch, capsys): From 25440aa83b16e254b102a307623c77291f954fd3 Mon Sep 17 00:00:00 2001 From: rafsanneloy Date: Tue, 14 Jul 2026 16:10:47 +0600 Subject: [PATCH 3/3] ruff check Signed-off-by: rafsanneloy --- cbrain_cli/cli_utils.py | 6 +----- cbrain_cli/handlers.py | 4 +--- tests/test_cli_utils_output.py | 15 +++------------ 3 files changed, 5 insertions(+), 20 deletions(-) diff --git a/cbrain_cli/cli_utils.py b/cbrain_cli/cli_utils.py index c0edbe4..5e1c00f 100644 --- a/cbrain_cli/cli_utils.py +++ b/cbrain_cli/cli_utils.py @@ -336,11 +336,7 @@ def confirm_destructive(args, prompt): if getattr(args, "yes", False): return True # JSON/JSONL must not mix with a prompt; pipes/EOF also never auto-confirm. - if ( - getattr(args, "json", False) - or getattr(args, "jsonl", False) - or not sys.stdin.isatty() - ): + if getattr(args, "json", False) or getattr(args, "jsonl", False) or not sys.stdin.isatty(): raise CliValidationError( "Refusing destructive action without confirmation; pass --yes", field="--yes", diff --git a/cbrain_cli/handlers.py b/cbrain_cli/handlers.py index f31b348..49436e0 100644 --- a/cbrain_cli/handlers.py +++ b/cbrain_cli/handlers.py @@ -120,9 +120,7 @@ def handle_dataprovider_is_alive(args): def handle_dataprovider_delete_unregistered(args): """Remove unregistered files from a data provider and display the cleanup results.""" - if not confirm_destructive( - args, f"Delete unregistered files on data provider {args.id}?" - ): + if not confirm_destructive(args, f"Delete unregistered files on data provider {args.id}?"): return 1 result = data_providers.delete_unregistered_files(args) if result is None: diff --git a/tests/test_cli_utils_output.py b/tests/test_cli_utils_output.py index 2a436dd..4c85559 100644 --- a/tests/test_cli_utils_output.py +++ b/tests/test_cli_utils_output.py @@ -116,19 +116,13 @@ def test_confirm_destructive_json_requires_yes(monkeypatch): def test_confirm_destructive_tty_accepts_y(monkeypatch): monkeypatch.setattr("sys.stdin.isatty", lambda: True) monkeypatch.setattr("builtins.input", lambda _: "y") - assert ( - confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") - is True - ) + assert confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") is True def test_confirm_destructive_tty_declines(monkeypatch, capsys): monkeypatch.setattr("sys.stdin.isatty", lambda: True) monkeypatch.setattr("builtins.input", lambda _: "n") - assert ( - confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") - is False - ) + assert confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") is False assert "Aborted." in capsys.readouterr().out @@ -139,8 +133,5 @@ def boom(_): raise EOFError monkeypatch.setattr("builtins.input", boom) - assert ( - confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") - is False - ) + assert confirm_destructive(MagicMock(yes=False, json=False, jsonl=False), "Delete?") is False assert "Aborted." in capsys.readouterr().out