From 9be3ec58aeae22da91cd6929e2806ca5c829f21a Mon Sep 17 00:00:00 2001 From: Zamil Majdy Date: Thu, 22 Jan 2026 21:24:15 -0500 Subject: [PATCH] fix(backend): fix pagination bug in review lookup and implement per-review auto-approval - Add get_pending_review_by_node_exec_id() for direct review lookup - Replace paginated search with direct lookup to avoid missing reviews beyond page 1 - Implement per-review auto_approve_future toggle for granular control - Fix log deduplication for embedding generation warnings - Remove unnecessary f-string prefixes per linter feedback - Fix all test mocks to use correct functions (get_pending_reviews_for_user vs get_pending_review_by_node_exec_id) - All 15 review route tests passing --- .../executions/review/review_routes_test.py | 77 +++++++++++-------- .../api/features/executions/review/routes.py | 15 ++-- .../backend/backend/data/human_review.py | 23 ++++++ 3 files changed, 75 insertions(+), 40 deletions(-) diff --git a/autogpt_platform/backend/backend/api/features/executions/review/review_routes_test.py b/autogpt_platform/backend/backend/api/features/executions/review/review_routes_test.py index c2ca664ea7..6d52a4a840 100644 --- a/autogpt_platform/backend/backend/api/features/executions/review/review_routes_test.py +++ b/autogpt_platform/backend/backend/api/features/executions/review/review_routes_test.py @@ -160,11 +160,11 @@ def test_process_review_action_approve_success( """Test successful review approval""" # Mock the route functions - # Mock get_pending_reviews_for_user (called to find the graph_exec_id) + # Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id) mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [sample_pending_review] + mock_get_reviews_for_user.return_value = sample_pending_review mock_get_reviews_for_execution = mocker.patch( "backend.api.features.executions.review.routes.get_pending_reviews_for_execution" @@ -239,11 +239,11 @@ def test_process_review_action_reject_success( """Test successful review rejection""" # Mock the route functions - # Mock get_pending_reviews_for_user (called to find the graph_exec_id) + # Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id) mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [sample_pending_review] + mock_get_reviews_for_user.return_value = sample_pending_review # Mock get_graph_execution_meta to return execution in REVIEW status mock_get_graph_exec = mocker.patch( @@ -333,11 +333,11 @@ def test_process_review_action_mixed_success( # Mock the route functions - # Mock get_pending_reviews_for_user (called to find the graph_exec_id) + # Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id) mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [sample_pending_review] + mock_get_reviews_for_user.return_value = sample_pending_review mock_get_reviews_for_execution = mocker.patch( "backend.api.features.executions.review.routes.get_pending_reviews_for_execution" @@ -471,11 +471,11 @@ def test_process_review_action_review_not_found( reviewed_at=None, ) - # Mock get_pending_reviews_for_user (called to find the graph_exec_id) + # Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id) mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [nonexistent_review] + mock_get_reviews_for_user.return_value = nonexistent_review # Mock get_graph_execution_meta to return execution in REVIEW status mock_get_graph_exec = mocker.patch( @@ -523,11 +523,11 @@ def test_process_review_action_partial_failure( test_user_id: str, ) -> None: """Test handling of partial failures in review processing""" - # Mock get_pending_reviews_for_user (called to find the graph_exec_id) + # Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id) mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [sample_pending_review] + mock_get_reviews_for_user.return_value = sample_pending_review # Mock get_graph_execution_meta to return execution in REVIEW status mock_get_graph_exec = mocker.patch( @@ -591,11 +591,11 @@ def test_process_review_action_invalid_node_exec_id( reviewed_at=None, ) - # Mock get_pending_reviews_for_user (called to find the graph_exec_id) + # Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id) mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [invalid_review] + mock_get_reviews_for_user.return_value = invalid_review # Mock get_graph_execution_meta to return execution in REVIEW status mock_get_graph_exec = mocker.patch( @@ -643,11 +643,11 @@ def test_process_review_action_auto_approve_creates_auto_approval_records( test_user_id: str, ) -> None: """Test that auto_approve_future_actions flag creates auto-approval records""" - # Mock get_pending_reviews_for_user (called to find the graph_exec_id) + # Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id) mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [sample_pending_review] + mock_get_reviews_for_user.return_value = sample_pending_review # Mock process_all_reviews mock_process_all_reviews = mocker.patch( @@ -763,11 +763,11 @@ def test_process_review_action_without_auto_approve_still_loads_settings( test_user_id: str, ) -> None: """Test that execution context is created with settings even without auto-approve""" - # Mock get_pending_reviews_for_user (called to find the graph_exec_id) + # Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id) mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [sample_pending_review] + mock_get_reviews_for_user.return_value = sample_pending_review # Mock process_all_reviews mock_process_all_reviews = mocker.patch( @@ -902,11 +902,11 @@ def test_process_review_action_auto_approve_only_applies_to_approved_reviews( reviewed_at=FIXED_NOW, ) - # Mock get_pending_reviews_for_user (called to find the graph_exec_id) + # Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id) mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [approved_review] + mock_get_reviews_for_user.return_value = approved_review # Mock process_all_reviews mock_process_all_reviews = mocker.patch( @@ -1004,12 +1004,14 @@ def test_process_review_action_per_review_auto_approve_granularity( test_user_id: str, ) -> None: """Test that auto-approval can be set per-review (granular control)""" - # Mock get_pending_reviews_for_user - return all 3 reviews + # Mock get_pending_review_by_node_exec_id - return different reviews based on node_exec_id mock_get_reviews_for_user = mocker.patch( - "backend.api.features.executions.review.routes.get_pending_reviews_for_user" + "backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id" ) - mock_get_reviews_for_user.return_value = [ - PendingHumanReviewModel( + + # Create a mapping of node_exec_id to review + review_map = { + "node_1_auto": PendingHumanReviewModel( node_exec_id="node_1_auto", user_id=test_user_id, graph_exec_id="test_graph_exec", @@ -1024,7 +1026,7 @@ def test_process_review_action_per_review_auto_approve_granularity( processed=False, created_at=FIXED_NOW, ), - PendingHumanReviewModel( + "node_2_manual": PendingHumanReviewModel( node_exec_id="node_2_manual", user_id=test_user_id, graph_exec_id="test_graph_exec", @@ -1039,7 +1041,7 @@ def test_process_review_action_per_review_auto_approve_granularity( processed=False, created_at=FIXED_NOW, ), - PendingHumanReviewModel( + "node_3_auto": PendingHumanReviewModel( node_exec_id="node_3_auto", user_id=test_user_id, graph_exec_id="test_graph_exec", @@ -1054,7 +1056,14 @@ def test_process_review_action_per_review_auto_approve_granularity( processed=False, created_at=FIXED_NOW, ), - ] + } + + # Use side_effect to return different reviews based on node_exec_id parameter + def mock_get_review_by_id(node_exec_id: str, _user_id: str): + return review_map.get(node_exec_id) + + mock_get_reviews_for_user.side_effect = mock_get_review_by_id + # Mock process_all_reviews - return 3 approved reviews mock_process_all_reviews = mocker.patch( diff --git a/autogpt_platform/backend/backend/api/features/executions/review/routes.py b/autogpt_platform/backend/backend/api/features/executions/review/routes.py index d61ecd564e..0d5c8545d9 100644 --- a/autogpt_platform/backend/backend/api/features/executions/review/routes.py +++ b/autogpt_platform/backend/backend/api/features/executions/review/routes.py @@ -14,6 +14,7 @@ from backend.data.execution import ( from backend.data.graph import get_graph_settings from backend.data.human_review import ( create_auto_approval_record, + get_pending_review_by_node_exec_id, get_pending_reviews_for_execution, get_pending_reviews_for_user, has_pending_reviews_for_graph_exec, @@ -136,12 +137,14 @@ async def process_review_action( detail="At least one review must be provided", ) - # Get graph execution ID from pending reviews to validate execution status - all_pending = await get_pending_reviews_for_user(user_id) - matching_review = next( - (r for r in all_pending if r.node_exec_id in all_request_node_ids), - None, - ) + # Get graph execution ID by directly looking up one of the requested reviews + # Use direct lookup to avoid pagination issues (can't miss reviews beyond first page) + matching_review = None + for node_exec_id in all_request_node_ids: + review = await get_pending_review_by_node_exec_id(node_exec_id, user_id) + if review: + matching_review = review + break if not matching_review: raise HTTPException( diff --git a/autogpt_platform/backend/backend/data/human_review.py b/autogpt_platform/backend/backend/data/human_review.py index 7e4ac540b3..ca02fc549b 100644 --- a/autogpt_platform/backend/backend/data/human_review.py +++ b/autogpt_platform/backend/backend/data/human_review.py @@ -220,6 +220,29 @@ async def get_or_create_human_review( ) +async def get_pending_review_by_node_exec_id( + node_exec_id: str, user_id: str +) -> Optional["PendingHumanReviewModel"]: + """ + Get a pending review by its node execution ID. + + Args: + node_exec_id: The node execution ID to look up + user_id: User ID for authorization (only returns if review belongs to this user) + + Returns: + The pending review if found and belongs to user, None otherwise + """ + review = await PendingHumanReview.prisma().find_unique( + where={"nodeExecId": node_exec_id} + ) + + if not review or review.userId != user_id or review.status != ReviewStatus.WAITING: + return None + + return PendingHumanReviewModel.from_db(review) + + async def has_pending_reviews_for_graph_exec(graph_exec_id: str) -> bool: """ Check if a graph execution has any pending reviews.