From a1219fee69c5c7f5353d06b31903de3e0bbc232e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 01:57:30 +0000 Subject: [PATCH 1/4] Initial plan From 95a9d2da855f3df894dcceb5444c620feccd9f50 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 02:08:25 +0000 Subject: [PATCH 2/4] Add verify_access_and_get_folders() to combine API access check and folder listing into a single request This implements the only valuable change from Jules PR #170 that wasn't already in main. Reduces startup API calls from 2 to 1, lowering latency. Updates sync_profile(), tests, and documentation accordingly. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com> --- WARP.md | 6 ++- main.py | 75 +++++++++++++++++++++++++++++++-- test_main.py | 53 +++++++++++++++-------- tests/test_parallel_deletion.py | 15 ++----- 4 files changed, 115 insertions(+), 34 deletions(-) diff --git a/WARP.md b/WARP.md index 7dbb25d..e40854b 100644 --- a/WARP.md +++ b/WARP.md @@ -91,7 +91,8 @@ The entire tool lives in `main.py` and is structured into clear phases: - `sanitize_for_log()` – Redacts `TOKEN` values from any log messages. 4. **Control D API helpers** - - `list_existing_folders()` – Returns a `{folder_name -> folder_id}` mapping for a profile. + - `verify_access_and_get_folders()` – Combines the API access check and fetching existing folders into a single request. Returns `{folder_name -> folder_id}` on success. + - `list_existing_folders()` – Helper that returns a `{folder_name -> folder_id}` mapping (used as fallback). - `get_all_existing_rules()` – Collects all existing rule PKs from both the root and each folder, using a `ThreadPoolExecutor` to parallelize per-folder fetches while accumulating into a shared `set` guarded by a lock. - `delete_folder()` – Deletes a folder by ID with error-logged failures. - `create_folder()` – Creates a folder and tries to read its ID directly from the response; if that fails, it polls `GET /groups` with increasing waits (using `FOLDER_CREATION_DELAY`) until the new folder appears. @@ -111,7 +112,8 @@ The entire tool lives in `main.py` and is structured into clear phases: 2. Builds a `plan_entry` summarizing folder names, rule counts, and per-action breakdown (for `rule_groups`), appending it to the shared `plan_accumulator`. 3. If `dry_run=True`, stops here after logging a summary message. 4. Otherwise, reuses a single `_api_client()` instance to: - - List and optionally delete existing folders with matching names (`--no-delete` skips this step). + - Verify access and list existing folders in one request (`verify_access_and_get_folders`). + - Optionally delete existing folders with matching names (`--no-delete` skips this step). - If any deletions occurred, waits ~60 seconds (`countdown_timer`) to let Control D fully process the removals. - Build the global `existing_rules` set. - Sequentially process each folder (executor with `max_workers=1` to avoid rate-limit and ordering issues), calling `_process_single_folder()` for each. diff --git a/main.py b/main.py index 787b95a..2ea85d2 100644 --- a/main.py +++ b/main.py @@ -718,6 +718,75 @@ def list_existing_folders(client: httpx.Client, profile_id: str) -> Dict[str, st return {} +def verify_access_and_get_folders( + client: httpx.Client, profile_id: str +) -> Optional[Dict[str, str]]: + """Combine access check and folder listing into a single API request. + + Returns: + Dict of {folder_name: folder_id} on success. + None if access is denied or the request fails after retries. + """ + url = f"{API_BASE}/{profile_id}/groups" + + for attempt in range(MAX_RETRIES): + try: + resp = client.get(url) + resp.raise_for_status() + + try: + data = resp.json() + folders = data.get("body", {}).get("groups", []) + return { + f["group"].strip(): f["PK"] + for f in folders + if f.get("group") and f.get("PK") + } + except (KeyError, ValueError) as e: + log.error(f"Failed to parse folders data: {sanitize_for_log(e)}") + return None + + except httpx.HTTPStatusError as e: + code = e.response.status_code + if code in (401, 403, 404): + if code == 401: + log.critical( + f"{Colors.FAIL}❌ Authentication Failed: The API Token is invalid.{Colors.ENDC}" + ) + log.critical( + f"{Colors.FAIL} Please check your token at: https://controld.com/account/manage-account{Colors.ENDC}" + ) + elif code == 403: + log.critical( + f"{Colors.FAIL}🚫 Access Denied: Token lacks permission for Profile {sanitize_for_log(profile_id)}.{Colors.ENDC}" + ) + elif code == 404: + log.critical( + f"{Colors.FAIL}🔍 Profile Not Found: The ID '{sanitize_for_log(profile_id)}' does not exist.{Colors.ENDC}" + ) + log.critical( + f"{Colors.FAIL} Please verify the Profile ID from your Control D Dashboard URL.{Colors.ENDC}" + ) + return None + + if attempt == MAX_RETRIES - 1: + log.error(f"API Request Failed ({code}): {sanitize_for_log(e)}") + return None + + except httpx.RequestError as e: + if attempt == MAX_RETRIES - 1: + log.error(f"Network Error: {sanitize_for_log(e)}") + return None + + wait_time = RETRY_DELAY * (2**attempt) + log.warning( + f"Request failed (attempt {attempt + 1}/{MAX_RETRIES}). Retrying in {wait_time}s..." + ) + time.sleep(wait_time) + + return None + + def get_all_existing_rules( client: httpx.Client, profile_id: str, @@ -1224,11 +1293,11 @@ def _fetch_if_valid(url: str): # Initial client for getting existing state AND processing folders # Optimization: Reuse the same client session to keep TCP connections alive with _api_client() as client: - # Check for API access problems first (401/403/404) - if not check_api_access(client, profile_id): + # Verify access and list existing folders in one request + existing_folders = verify_access_and_get_folders(client, profile_id) + if existing_folders is None: return False - existing_folders = list_existing_folders(client, profile_id) if not no_delete: deletion_occurred = False diff --git a/test_main.py b/test_main.py index e15c805..599ee4a 100644 --- a/test_main.py +++ b/test_main.py @@ -248,16 +248,27 @@ def test_interactive_prompts_show_hints(monkeypatch, capsys): assert "https://controld.com/account/manage-account" in stdout -# Case 7: check_api_access handles success and errors correctly -def test_check_api_access_success(monkeypatch): +# Case 7: verify_access_and_get_folders handles success and errors correctly +def test_verify_access_and_get_folders_success(monkeypatch): m = reload_main_with_env(monkeypatch) mock_client = MagicMock() - mock_client.get.return_value.raise_for_status.return_value = None - - assert m.check_api_access(mock_client, "valid_profile") is True - - -def test_check_api_access_401(monkeypatch): + mock_response = MagicMock() + mock_response.json.return_value = { + "body": { + "groups": [ + {"group": "Folder A", "PK": "id_a"}, + {"group": "Folder B", "PK": "id_b"} + ] + } + } + mock_client.get.return_value = mock_response + mock_response.raise_for_status.return_value = None + + result = m.verify_access_and_get_folders(mock_client, "valid_profile") + assert result == {"Folder A": "id_a", "Folder B": "id_b"} + + +def test_verify_access_and_get_folders_401(monkeypatch): m = reload_main_with_env(monkeypatch) mock_client = MagicMock() @@ -273,14 +284,14 @@ def test_check_api_access_401(monkeypatch): mock_log = MagicMock() monkeypatch.setattr(m, "log", mock_log) - assert m.check_api_access(mock_client, "invalid_token") is False + assert m.verify_access_and_get_folders(mock_client, "invalid_token") is None assert mock_log.critical.call_count >= 1 # Check for authentication failed message args = str(mock_log.critical.call_args_list) assert "Authentication Failed" in args -def test_check_api_access_403(monkeypatch): +def test_verify_access_and_get_folders_403(monkeypatch): m = reload_main_with_env(monkeypatch) mock_client = MagicMock() @@ -295,12 +306,12 @@ def test_check_api_access_403(monkeypatch): mock_log = MagicMock() monkeypatch.setattr(m, "log", mock_log) - assert m.check_api_access(mock_client, "forbidden_profile") is False + assert m.verify_access_and_get_folders(mock_client, "forbidden_profile") is None assert mock_log.critical.call_count == 1 assert "Access Denied" in str(mock_log.critical.call_args) -def test_check_api_access_404(monkeypatch): +def test_verify_access_and_get_folders_404(monkeypatch): m = reload_main_with_env(monkeypatch) mock_client = MagicMock() @@ -315,12 +326,12 @@ def test_check_api_access_404(monkeypatch): mock_log = MagicMock() monkeypatch.setattr(m, "log", mock_log) - assert m.check_api_access(mock_client, "missing_profile") is False + assert m.verify_access_and_get_folders(mock_client, "missing_profile") is None assert mock_log.critical.call_count >= 1 assert "Profile Not Found" in str(mock_log.critical.call_args_list) -def test_check_api_access_generic_http_error(monkeypatch): +def test_verify_access_and_get_folders_500_retry(monkeypatch): m = reload_main_with_env(monkeypatch) mock_client = MagicMock() @@ -334,13 +345,17 @@ def test_check_api_access_generic_http_error(monkeypatch): mock_log = MagicMock() monkeypatch.setattr(m, "log", mock_log) + monkeypatch.setattr(m, "RETRY_DELAY", 0.001) + monkeypatch.setattr("time.sleep", lambda x: None) + monkeypatch.setattr(m, "MAX_RETRIES", 2) - assert m.check_api_access(mock_client, "profile") is False + assert m.verify_access_and_get_folders(mock_client, "profile") is None + assert mock_client.get.call_count == 2 assert mock_log.error.called assert "500" in str(mock_log.error.call_args) -def test_check_api_access_network_error(monkeypatch): +def test_verify_access_and_get_folders_network_error(monkeypatch): m = reload_main_with_env(monkeypatch) mock_client = MagicMock() @@ -350,8 +365,12 @@ def test_check_api_access_network_error(monkeypatch): mock_log = MagicMock() monkeypatch.setattr(m, "log", mock_log) + monkeypatch.setattr(m, "RETRY_DELAY", 0.001) + monkeypatch.setattr("time.sleep", lambda x: None) + monkeypatch.setattr(m, "MAX_RETRIES", 2) - assert m.check_api_access(mock_client, "profile") is False + assert m.verify_access_and_get_folders(mock_client, "profile") is None + assert mock_client.get.call_count == 2 assert mock_log.error.called assert "Network failure" in str(mock_log.error.call_args) diff --git a/tests/test_parallel_deletion.py b/tests/test_parallel_deletion.py index bb0fef4..33d22b1 100644 --- a/tests/test_parallel_deletion.py +++ b/tests/test_parallel_deletion.py @@ -36,10 +36,7 @@ def test_parallel_deletion_uses_threadpool(mock_env, monkeypatch): __enter__=lambda self: mock_client, __exit__=lambda *args: None ) monkeypatch.setattr(main, "_api_client", lambda: mock_client_ctx) - monkeypatch.setattr(main, "check_api_access", lambda *args: True) - monkeypatch.setattr( - main, "list_existing_folders", lambda *args: {"FolderA": "id1", "FolderB": "id2"} - ) + monkeypatch.setattr(main, "verify_access_and_get_folders", lambda *args: {"FolderA": "id1", "FolderB": "id2"}) monkeypatch.setattr(main, "delete_folder", lambda *args: True) monkeypatch.setattr(main, "get_all_existing_rules", lambda *args: set()) monkeypatch.setattr(main, "countdown_timer", lambda *args: None) @@ -93,10 +90,7 @@ def test_parallel_deletion_handles_exceptions(mock_env, monkeypatch): __enter__=lambda self: mock_client, __exit__=lambda *args: None ) monkeypatch.setattr(main, "_api_client", lambda: mock_client_ctx) - monkeypatch.setattr(main, "check_api_access", lambda *args: True) - monkeypatch.setattr( - main, "list_existing_folders", lambda *args: {"Folder1": "id1"} - ) + monkeypatch.setattr(main, "verify_access_and_get_folders", lambda *args: {"Folder1": "id1"}) # Mock delete_folder to raise an exception def failing_delete(*args): @@ -146,10 +140,7 @@ def test_parallel_deletion_sanitizes_exception(mock_env, monkeypatch): __enter__=lambda self: mock_client, __exit__=lambda *args: None ) monkeypatch.setattr(main, "_api_client", lambda: mock_client_ctx) - monkeypatch.setattr(main, "check_api_access", lambda *args: True) - monkeypatch.setattr( - main, "list_existing_folders", lambda *args: {"TestFolder": "id1"} - ) + monkeypatch.setattr(main, "verify_access_and_get_folders", lambda *args: {"TestFolder": "id1"}) # Mock delete_folder to raise exception with potentially dangerous content def failing_delete(*args): From 3628c87efb8f86182eb781ac9b57fad756ec865c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 02:09:35 +0000 Subject: [PATCH 3/4] Address code review feedback: improve error message clarity and add fallthrough logging Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com> --- main.py | 5 ++++- test_main.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/main.py b/main.py index 2ea85d2..cf8a944 100644 --- a/main.py +++ b/main.py @@ -775,7 +775,9 @@ def verify_access_and_get_folders( except httpx.RequestError as e: if attempt == MAX_RETRIES - 1: - log.error(f"Network Error: {sanitize_for_log(e)}") + log.error( + f"Network error during access verification: {sanitize_for_log(e)}" + ) return None wait_time = RETRY_DELAY * (2**attempt) @@ -784,6 +786,7 @@ def verify_access_and_get_folders( ) time.sleep(wait_time) + log.error("Access verification failed after all retries") return None diff --git a/test_main.py b/test_main.py index 599ee4a..6ab96bc 100644 --- a/test_main.py +++ b/test_main.py @@ -372,7 +372,7 @@ def test_verify_access_and_get_folders_network_error(monkeypatch): assert m.verify_access_and_get_folders(mock_client, "profile") is None assert mock_client.get.call_count == 2 assert mock_log.error.called - assert "Network failure" in str(mock_log.error.call_args) + assert "Network error" in str(mock_log.error.call_args) or "access verification" in str(mock_log.error.call_args) # Case 8: extract_profile_id correctly extracts ID from URL or returns input From 14c2f8a5c78bc3d3843a22142ed5f7c0873b4799 Mon Sep 17 00:00:00 2001 From: Abhi Mehrotra Date: Sun, 8 Feb 2026 20:29:16 -0600 Subject: [PATCH 4/4] Update main.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- main.py | 52 +++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 7 deletions(-) diff --git a/main.py b/main.py index cf8a944..09f5337 100644 --- a/main.py +++ b/main.py @@ -736,13 +736,51 @@ def verify_access_and_get_folders( try: data = resp.json() - folders = data.get("body", {}).get("groups", []) - return { - f["group"].strip(): f["PK"] - for f in folders - if f.get("group") and f.get("PK") - } - except (KeyError, ValueError) as e: + + # Ensure we got the expected top-level JSON structure. + # We defensively validate types here so that unexpected but valid + # JSON (e.g., a list or a scalar) doesn't cause AttributeError/TypeError + # and crash the sync logic. + if not isinstance(data, dict): + log.error( + "Failed to parse folders data: expected JSON object at top level, " + f"got {type(data).__name__}" + ) + return None + + body = data.get("body") + if not isinstance(body, dict): + log.error( + "Failed to parse folders data: expected 'body' to be an object, " + f"got {type(body).__name__ if body is not None else 'None'}" + ) + return None + + folders = body.get("groups", []) + if not isinstance(folders, list): + log.error( + "Failed to parse folders data: expected 'body[\"groups\"]' to be a list, " + f"got {type(folders).__name__}" + ) + return None + + # Only process entries that are dicts and have the required keys. + result: Dict[str, str] = {} + for f in folders: + if not isinstance(f, dict): + # Skip non-dict entries instead of crashing; this protects + # against partial data corruption or unexpected API changes. + continue + name = f.get("group") + pk = f.get("PK") + if not name or not pk: + continue + result[str(name).strip()] = str(pk) + + return result + except (KeyError, ValueError, TypeError, AttributeError) as e: + # As a final safeguard, catch any remaining parsing/shape errors so + # that a malformed response cannot crash the caller. log.error(f"Failed to parse folders data: {sanitize_for_log(e)}") return None