🧹 Refactor: Simplify create_folder logic#793
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
PR SummaryMedium Risk Overview Also replaces the contents of Reviewed by Cursor Bugbot for commit 5b770ab. Configure here. |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| def _poll_for_folder_id(ctx: SyncContext, name: str) -> str | None: | ||
| for attempt in range(MAX_RETRIES + 1): | ||
| try: | ||
| data = _api_get(ctx.client, f"{API_BASE}/{ctx.profile_id}/groups").json() | ||
| groups = data.get("body", {}).get("groups", []) | ||
|
|
||
| for grp in groups: | ||
| if grp["group"].strip() == name.strip(): | ||
| pk = str(grp["PK"]) | ||
| if not validate_folder_id(pk, log_errors=False): | ||
| log.error( | ||
| f"API returned invalid folder ID: {sanitize_for_log(pk)}" | ||
| ) | ||
| return None | ||
| log.info( | ||
| "Created folder %s (ID %s) [Polled]", | ||
| sanitize_for_log(name), | ||
| sanitize_for_log(pk), | ||
| ) | ||
| return pk | ||
| except Exception as e: | ||
| log.warning( | ||
| f"Error fetching groups on attempt {attempt}: {sanitize_for_log(e)}" | ||
| ) | ||
|
|
||
| if attempt < MAX_RETRIES: | ||
| wait_time = FOLDER_CREATION_DELAY * (attempt + 1) | ||
| log.info( | ||
| f"Folder '{sanitize_for_log(name)}' not found yet. Retrying in {wait_time}s..." | ||
| ) | ||
| time.sleep(wait_time) | ||
|
|
||
| log.error( | ||
| f"Folder {sanitize_for_log(name)} was not found after creation and retries." | ||
| ) | ||
| return None |
There was a problem hiding this comment.
📝 Info: Exception propagation semantics preserved for _poll_for_folder_id
I checked whether extracting the polling loop into _poll_for_folder_id changed exception handling. The inner except Exception at main.py:2124 catches all standard exceptions (including httpx.HTTPError and KeyError), which means the outer except (httpx.HTTPError, KeyError) in create_folder at main.py:2163 was already unreachable from the polling path in the original code. So the extraction doesn't change exception semantics for that path.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Invalid folder ID now triggers slow polling instead of failing fast
- _extract_folder_id_from_response now returns (id, poll_if_missing) so an invalid single-group PK sets poll_if_missing=False and create_folder returns None without calling _poll_for_folder_id.
- ✅ Fixed: Debugging script committed in test temporary file
- Replaced the two-line import/print script with the same dotenv mock_load_dotenv bootstrap and main import as test_main.py.
Preview (4ae717aee2)
diff --git a/main.py b/main.py
--- a/main.py
+++ b/main.py
@@ -2063,6 +2063,84 @@
return False
+def _extract_folder_id_from_response(
+ response: httpx.Response, name: str
+) -> tuple[str | None, bool]:
+ try:
+ resp_data = response.json()
+ body = resp_data.get("body", {})
+
+ if isinstance(body, dict) and "group" in body and "PK" in body["group"]:
+ pk = str(body["group"]["PK"])
+ if not validate_folder_id(pk, log_errors=False):
+ log.error(f"API returned invalid folder ID: {sanitize_for_log(pk)}")
+ return None, False
+ log.info(
+ "Created folder %s (ID %s) [Direct]",
+ sanitize_for_log(name),
+ sanitize_for_log(pk),
+ )
+ return pk, False
+
+ if isinstance(body, dict) and "groups" in body:
+ for grp in body["groups"]:
+ if grp.get("group") == name:
+ pk = str(grp["PK"])
+ if not validate_folder_id(pk, log_errors=False):
+ log.error(
+ f"API returned invalid folder ID: {sanitize_for_log(pk)}"
+ )
+ continue
+ log.info(
+ "Created folder %s (ID %s) [Direct]",
+ sanitize_for_log(name),
+ sanitize_for_log(pk),
+ )
+ return pk, False
+ except Exception as e:
+ if log.isEnabledFor(logging.DEBUG):
+ log.debug(f"Could not extract ID from POST response: {sanitize_for_log(e)}")
+ return None, True
+
+
+def _poll_for_folder_id(ctx: SyncContext, name: str) -> str | None:
+ for attempt in range(MAX_RETRIES + 1):
+ try:
+ data = _api_get(ctx.client, f"{API_BASE}/{ctx.profile_id}/groups").json()
+ groups = data.get("body", {}).get("groups", [])
+
+ for grp in groups:
+ if grp["group"].strip() == name.strip():
+ pk = str(grp["PK"])
+ if not validate_folder_id(pk, log_errors=False):
+ log.error(
+ f"API returned invalid folder ID: {sanitize_for_log(pk)}"
+ )
+ return None
+ log.info(
+ "Created folder %s (ID %s) [Polled]",
+ sanitize_for_log(name),
+ sanitize_for_log(pk),
+ )
+ return pk
+ except Exception as e:
+ log.warning(
+ f"Error fetching groups on attempt {attempt}: {sanitize_for_log(e)}"
+ )
+
+ if attempt < MAX_RETRIES:
+ wait_time = FOLDER_CREATION_DELAY * (attempt + 1)
+ log.info(
+ f"Folder '{sanitize_for_log(name)}' not found yet. Retrying in {wait_time}s..."
+ )
+ time.sleep(wait_time)
+
+ log.error(
+ f"Folder {sanitize_for_log(name)} was not found after creation and retries."
+ )
+ return None
+
+
def create_folder(ctx: SyncContext, name: str, action: RuleAction) -> str | None:
"""
Create a new folder and return its ID.
@@ -2077,84 +2155,15 @@
)
# OPTIMIZATION: Try to grab ID directly from response to avoid the wait loop
- try:
- resp_data = response.json()
- body = resp_data.get("body", {})
+ pk, poll_if_missing = _extract_folder_id_from_response(response, name)
+ if pk:
+ return pk
+ if not poll_if_missing:
+ return None
- # Check if it returned a single group object
- if isinstance(body, dict) and "group" in body and "PK" in body["group"]:
- pk = str(body["group"]["PK"])
- if not validate_folder_id(pk, log_errors=False):
- log.error(f"API returned invalid folder ID: {sanitize_for_log(pk)}")
- return None
- log.info(
- "Created folder %s (ID %s) [Direct]",
- sanitize_for_log(name),
- sanitize_for_log(pk),
- )
- return pk
-
- # Check if it returned a list containing our group
- if isinstance(body, dict) and "groups" in body:
- for grp in body["groups"]:
- if grp.get("group") == name:
- pk = str(grp["PK"])
- if not validate_folder_id(pk, log_errors=False):
- log.error(
- f"API returned invalid folder ID: {sanitize_for_log(pk)}"
- )
- continue
- log.info(
- "Created folder %s (ID %s) [Direct]",
- sanitize_for_log(name),
- sanitize_for_log(pk),
- )
- return pk
- except Exception as e:
- if log.isEnabledFor(logging.DEBUG):
- log.debug(
- f"Could not extract ID from POST response: {sanitize_for_log(e)}"
- )
-
# 2. Fallback: Poll for the new folder (The Robust Retry Logic)
- for attempt in range(MAX_RETRIES + 1):
- try:
- data = _api_get(
- ctx.client, f"{API_BASE}/{ctx.profile_id}/groups"
- ).json()
- groups = data.get("body", {}).get("groups", [])
+ return _poll_for_folder_id(ctx, name)
- for grp in groups:
- if grp["group"].strip() == name.strip():
- pk = str(grp["PK"])
- if not validate_folder_id(pk, log_errors=False):
- log.error(
- f"API returned invalid folder ID: {sanitize_for_log(pk)}"
- )
- return None
- log.info(
- "Created folder %s (ID %s) [Polled]",
- sanitize_for_log(name),
- sanitize_for_log(pk),
- )
- return pk
- except Exception as e:
- log.warning(
- f"Error fetching groups on attempt {attempt}: {sanitize_for_log(e)}"
- )
-
- if attempt < MAX_RETRIES:
- wait_time = FOLDER_CREATION_DELAY * (attempt + 1)
- log.info(
- f"Folder '{sanitize_for_log(name)}' not found yet. Retrying in {wait_time}s..."
- )
- time.sleep(wait_time)
-
- log.error(
- f"Folder {sanitize_for_log(name)} was not found after creation and retries."
- )
- return None
-
except (httpx.HTTPError, KeyError) as e:
hint = ""
if isinstance(e, httpx.HTTPStatusError):
diff --git a/test_main.py.tmp b/test_main.py.tmp
--- a/test_main.py.tmp
+++ b/test_main.py.tmp
@@ -1,10 +1,18 @@
+import importlib
+import os
+import sys
+from unittest.mock import MagicMock, patch
+
import dotenv
import httpx
import pytest
from typing import Any
+
def mock_load_dotenv(*args: Any, **kwargs: Any) -> bool:
return True
+
dotenv.load_dotenv = mock_load_dotenv
+import main # noqa: E402You can send follow-ups to the cloud agent here.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5b770ab. Configure here.
| ) | ||
| pk = _extract_folder_id_from_response(response, name) | ||
| if pk: | ||
| return pk |
There was a problem hiding this comment.
Invalid folder ID now triggers slow polling instead of failing fast
Medium Severity
In the original code, when the API response contained body.group.PK with an invalid folder ID, create_folder returned None immediately (fast fail). After the refactoring, _extract_folder_id_from_response returns None for this case, but the caller treats it the same as "no ID found" via if pk:, falling through to _poll_for_folder_id. This turns a fast failure into ~275 seconds of unnecessary polling (MAX_RETRIES=10 with escalating delays). The None return from _extract_folder_id_from_response is ambiguous — it conflates "extraction not possible" with "API returned an invalid ID."
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5b770ab. Configure here.
|
|
||
| dotenv.load_dotenv = mock_load_dotenv | ||
| from main import _extract_folder_id_from_response, _poll_for_folder_id, create_folder | ||
| print(f"Functions imported successfully") |
There was a problem hiding this comment.
Debugging script committed in test temporary file
Low Severity
The test_main.py.tmp file was replaced with a manual verification script containing a bare print(f"Functions imported successfully") statement. This is clearly a debugging/smoke-test artifact used to verify the new helper functions can be imported, not meaningful test or production code.
Reviewed by Cursor Bugbot for commit 5b770ab. Configure here.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| group = body.get("group") | ||
| if isinstance(group, dict) and "PK" in group: | ||
| return _process_new_folder_pk(str(group["PK"]), name, "Direct") |
There was a problem hiding this comment.
📝 Info: Improved type safety in _extract_folder_id_from_response
The old code checked "PK" in body["group"] without verifying that body["group"] is a dict. If the API returned body = {"group": "some_string_containing_PK"}, the old code would pass the in check (substring match on a string), then fail with a TypeError on body["group"]["PK"] (caught by the outer except Exception). The new code at line 2086 adds isinstance(group, dict), which is a genuine defensive improvement that prevents this subtle edge case from even reaching the exception handler.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for grp in groups: | ||
| if grp.get("group", "").strip() == name.strip() and "PK" in grp: |
There was a problem hiding this comment.
📝 Info: Safer key access in _poll_for_folder_id vs old polling code
The old polling code used grp["group"].strip() which would raise KeyError if a group entry lacked the "group" key (caught by the inner except Exception). The new code at line 2109 uses grp.get("group", "").strip() which silently handles missing keys. This is a minor behavioral improvement — the old code would log a warning for each such malformed entry (via the exception handler), while the new code silently skips them. Both ultimately continue polling, so the end result is the same.
Was this helpful? React with 👍 or 👎 to provide feedback.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
abhimehro
force-pushed
the
jules-1746748579011273344-85e156a2
branch
from
7873bea to
3896cf9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| def _extract_folder_id_from_response(response: httpx.Response, name: str) -> str | None: | ||
| try: | ||
| body = response.json().get("body") | ||
| except Exception as e: | ||
| if log.isEnabledFor(logging.DEBUG): | ||
| log.debug(f"Could not extract ID from POST response: {sanitize_for_log(e)}") | ||
| return None | ||
|
|
||
| if not isinstance(body, dict): | ||
| return None | ||
|
|
||
| group = body.get("group") | ||
| if isinstance(group, dict) and "PK" in group: | ||
| return _process_new_folder_pk(str(group["PK"]), name, "Direct") | ||
|
|
||
| groups = body.get("groups") | ||
| if isinstance(groups, list): | ||
| return _extract_from_groups_list(groups, name) | ||
|
|
||
| return None |
There was a problem hiding this comment.
📝 Info: Exception handling scope narrowed from broad except Exception to targeted catch
In the old code, the entire response-parsing block (JSON deserialization, group matching, PK validation, logging) was wrapped in a single except Exception handler. Any unexpected exception would be caught, debug-logged, and execution would fall through to the polling path.
In the new _extract_folder_id_from_response, only response.json().get("body") at line 2115 is wrapped in except Exception. The subsequent calls to _process_new_folder_pk and _extract_from_groups_list (lines 2124-2134) are not exception-guarded. If those helpers raised an unexpected exception, it would propagate to create_folder's outer except (httpx.HTTPError, KeyError) at main.py:2194, which only catches those two types — other exception types would propagate uncaught.
In practice this is safe because the helper code is carefully written with isinstance guards and key-existence checks, and JSON-deserialized values always support str(). But it's a semantic difference worth being aware of for future maintenance.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _extract_from_groups_list(groups: list, name: str) -> str | None: | ||
| for grp in groups: | ||
| if isinstance(grp, dict) and grp.get("group") == name and "PK" in grp: | ||
| pk = _process_new_folder_pk(str(grp["PK"]), name, "Direct") | ||
| if pk: | ||
| return pk | ||
| return None |
There was a problem hiding this comment.
📝 Info: Improved defensive checks in _extract_from_groups_list vs old code
The old groups list iteration accessed grp["PK"] directly without checking for key existence first, relying on the outer except Exception to catch KeyError. The new _extract_from_groups_list at main.py:2081 adds isinstance(grp, dict) and "PK" in grp guards before accessing grp["PK"]. Similarly, _poll_for_folder_id at main.py:2117 uses grp.get("group", "") instead of grp["group"], avoiding potential KeyError. These are genuine improvements to robustness, not behavioral regressions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if attempt < MAX_RETRIES: | ||
| wait_time = FOLDER_CREATION_DELAY * (attempt + 1) | ||
| log.info( | ||
| f"Folder '{sanitize_for_log(name)}' not found yet. Retrying in {wait_time}s..." | ||
| ) | ||
| time.sleep(wait_time) |
There was a problem hiding this comment.
📝 Info: Polling time.sleep is outside try/except — same as old code
In _poll_for_folder_id at main.py:2133-2138, the if attempt < MAX_RETRIES block (including time.sleep) sits outside the try/except Exception block. If time.sleep were interrupted by a signal that raises an exception (very unlikely in normal operation), it would propagate up through create_folder's except (httpx.HTTPError, KeyError) handler, potentially uncaught. This is identical to the old code's structure and is not a new concern, but worth noting that the extraction didn't change this characteristic.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| name_stripped = name.strip() | ||
| for grp in groups: | ||
| if ( | ||
| isinstance(grp, dict) | ||
| and isinstance(grp.get("group"), str) | ||
| and grp["group"].strip() == name_stripped | ||
| and "PK" in grp |
There was a problem hiding this comment.
📝 Info: Name matching changed from exact to stripped comparison for response parsing path
The old response-parsing code used grp.get("group") == name (exact match) for the groups-list path, while the polling path used .strip() comparison. The new _extract_from_groups_list uses grp["group"].strip() == name_stripped (line 2093), making both paths consistent. This is a behavioral change: a group name with leading/trailing whitespace will now match during response parsing where it previously wouldn't (and would have fallen through to polling). This is an improvement that eliminates an inconsistency between the two code paths.
Was this helpful? React with 👍 or 👎 to provide feedback.
abhimehro
force-pushed
the
jules-1746748579011273344-85e156a2
branch
from
0739733 to
5333f9a
Compare
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| main.py | 1.66 → 1.70 | Lines of Code in a Single File, Complex Method, Complex Conditional, Bumpy Road Ahead, Overall Code Complexity, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
🎯 What: Extracted
_extract_folder_id_from_responseand_poll_for_folder_idhelper functions fromcreate_folder.💡 Why: Reduces cyclomatic complexity and deeply nested try-except blocks, fixing a CodeScene code health issue.
✅ Verification: Ran formatting, linting, and full pytest suite.
✨ Result: Improved maintainability and readability.
PR created automatically by Jules for task 1746748579011273344 started by @abhimehro