Repository navigation
Wegent/refactor task subtask store boundary - #1370
Merged
qdaxb merged 5 commits intoJun 12, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughStore-layer task/subtask access is introduced and wired across backend endpoints, services, WebSocket handlers, executors, and tests; ORM queries and in-place JSON/subtask mutations are replaced by task_store, subtask_store, and task_access_store calls. ChangesStore-backed task and subtask access
Estimated code review effort:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/tests/services/subscription/test_task_completion_handler.py (1)
45-64:⚠️ Potential issue | 🟠 MajorFix test coupling: mock store collaborators, not
db.queryORM wiringEven though
SubscriptionTaskCompletionHandlercallstask_store.get_task_by_states(...)/subtask_store.list_not_executor_deleted_by_task(...),backend/tests/services/subscription/test_task_completion_handler.pystill mocksdb.query(...)with branches forKind,TaskResource, andSubtask(45-64, 163-180, 321-340, 431-461). This ties the tests to the stores’ current ORM query shapes and won’t catch a regression where the handler starts doing direct CRUD onTaskResource/Subtaskagain.Adjust these tests to patch/assert the
task_store/subtask_storemethods (andupdate_fields) rather than stubbingdb.query(TaskResource|Subtask).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/subscription/test_task_completion_handler.py` around lines 45 - 64, The tests currently mock db.query for Kind/TaskResource/Subtask; instead patch the stores the handler uses: replace db.query stubs with mocks of task_store.get_task_by_states(...) and subtask_store.list_not_executor_deleted_by_task(...) (and any subtask_store.update_fields / task_store.update_fields calls) so the test exercises SubscriptionTaskCompletionHandler via its collaborators; assert those store methods are called with expected args and return the prepared task/subtask objects rather than branching on ORM models like TaskResource/Subtask/Kind.backend/app/mcp_server/tools/subscription.py (1)
117-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the team with owner context, not just name and namespace.
After loading the task, this helper still looks up the
Teamby(kind, name, namespace)only. For personal teams, that is not unique, so duplicate team names across users can route the subscription to the wrongteam_id/model. Use the task owner's context when resolving the team instead of a bareKindquery here.As per coding guidelines, "A Kind resource is uniquely identified by THREE fields:
namespace,name, anduser_id- always use all three fields to locate a Kind".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/mcp_server/tools/subscription.py` around lines 117 - 145, The current lookup for the Team Kind after loading the task uses only Kind.kind/Kind.name/Kind.namespace which can return the wrong Team for personal teams; update the query that resolves `team` (the block using db.query(Kind).filter(...).first()) to also filter by the task owner’s user id (use the owner id from the loaded `task` JSON / metadata) so the Kind lookup uses the three identifying fields: namespace, name, and user_id; keep the same extraction of modelRef (`model_ref = team.json.get("spec", {}).get("modelRef", {})`) and model_name/namespace assignment but ensure the filter includes `Kind.user_id == <task owner id>` to uniquely resolve the Team.Source: Coding guidelines
backend/app/services/adapters/team_kinds.py (1)
816-824:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGroup-team renames only rewrite the acting user's tasks.
update_with_user()allows any Developer in a group namespace to rename the team, but_update_team_references_in_tasks()now only loadstask_store.list_regular_active_tasks(db, user_id=user_id). When the editor is not the owner of every task in that group, other members' task JSON keeps the oldteamRef, so the rename leaves the group in an inconsistent state.Also applies to: 866-868, 1873-1891
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/adapters/team_kinds.py` around lines 816 - 824, The rename currently only updates tasks owned by the acting user because _update_team_references_in_tasks() calls task_store.list_regular_active_tasks(db, user_id=user_id); change this so group-team renames enumerate all tasks in the group namespace (either by calling the task store without the user_id filter or by querying tasks for all users in the group/namespace) and update every task's teamRef, not just the editor's. Update _update_team_references_in_tasks() (and the other call sites noted around update_with_user(), lines referenced 866-868 and 1873-1891) to accept/derive the group/namespace and fetch/update tasks across the full namespace so the rename is applied consistently for all members. Ensure existing permission checks (check_group_permission and Developer role) remain unchanged.backend/app/services/adapters/collaboration_strategy.py (1)
218-226:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the
Teamlookup byuser_idat both call sites.These queries no longer use the full Kind identity. If two users have a
Teamwith the samenamespaceandname,.first()can bind the wrong team here, which changes the collaboration model and pipeline membership resolution for the task.As per coding guidelines, "A Kind resource is uniquely identified by THREE fields:
namespace,name, anduser_id- always use all three fields to locate a Kind."🔧 Apply the owner scope consistently
- team = ( - db.query(Kind) - .filter( - Kind.kind == "Team", - Kind.name == team_ref.name, - Kind.namespace == team_ref.namespace, - Kind.is_active.is_(True), - ) - .first() - ) + team = ( + db.query(Kind) + .filter( + Kind.kind == "Team", + Kind.user_id == task.user_id, + Kind.name == team_ref.name, + Kind.namespace == team_ref.namespace, + Kind.is_active.is_(True), + ) + .first() + )Apply the same fix at both sites, or replace both queries with the existing
kindReader.get_by_name_and_namespace(...)helper.Also applies to: 355-363
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/adapters/collaboration_strategy.py` around lines 218 - 226, The Team lookup currently filters only by Kind.kind, Kind.name and Kind.namespace and can return the wrong Team when multiple users have identical namespace/name; update both call sites (the query at the shown block and the similar one at lines ~355-363) to include the owner scope by adding a filter for Kind.user_id == team_ref.user_id (or, better, replace both raw queries with the existing helper kindReader.get_by_name_and_namespace(team_ref.name, team_ref.namespace, team_ref.user_id) to ensure all three identity fields (namespace, name, user_id) are used when resolving a Team).Source: Coding guidelines
backend/app/services/inbox/direct_agent_handler.py (1)
255-278:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid capping the candidate task set before the team match.
list_regular_active_tasks(..., limit=50)truncates the user's active tasks globally, and only then do we filter byteamRef. If the newest task for this team is older than those 50 rows,_find_latest_workspace_params()returns{}and direct-agent creation silently falls back to a plain chat task with no workspace/repo context. Please push the team filter into the store query or page until a matching team task is found.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/inbox/direct_agent_handler.py` around lines 255 - 278, The code currently calls task_store.list_regular_active_tasks(..., limit=50) then filters by teamRef, which can miss older team tasks; update _find_latest_workspace_params to either (1) extend list_regular_active_tasks to accept team filters (name/namespace) and pass team.name and team.namespace so the DB query restricts by teamRef, or (2) implement pagination in _find_latest_workspace_params: repeatedly call list_regular_active_tasks with an offset/page token until a matching team_ref is found (using task_resource.json->spec->teamRef) and only then call task_store.get_workspace_by_ref; modify references to list_regular_active_tasks and latest_task accordingly to ensure you don’t silently fall back when a matching team task exists beyond the initial 50 rows.
🧹 Nitpick comments (10)
backend/tests/core/test_security.py (1)
356-382: ⚡ Quick winCover the non-admin impersonation denial path.
These tests only prove the happy path. Please add the case where a non-admin service key supplies another user's
wegent_username; that is the branch that would turn this change into user impersonation if it regresses. As per coding guidelines, "Test edge cases and error conditions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/core/test_security.py` around lines 356 - 382, Add a negative test named like test_non_admin_cannot_impersonate_other_user that uses _create_service_key to create a service key owned by user A, then calls get_auth_context with that key but passing wegent_username of a different user B; assert the call is rejected (preferably with pytest.raises(the project's authorization/permission exception) or, if code returns a context, assert auth_context.user.id != user B.id and equals the key owner's id). This ensures the non-admin impersonation denial branch for get_auth_context is covered.Source: Coding guidelines
backend/app/api/endpoints/internal/callback.py (1)
111-215: ⚡ Quick winInstrument the callback entrypoints with the required tracing decorators.
These async request handlers are part of a high-traffic execution path, but the updated code still doesn't apply
@trace_async()or record task/subtask span metadata. That makes the new store-backed lookup harder to debug in production when callbacks misroute or stall.Suggested direction
+from shared.telemetry.decorators import trace_async +from shared.telemetry import add_span_event, set_span_attribute ... +@trace_async() `@router.post`("", response_model=CallbackResponse) async def handle_callback( request: CallbackRequest, db: Session = Depends(get_db), ) -> CallbackResponse: + set_span_attribute('task.id', request.task_id) + set_span_attribute('subtask.id', request.subtask_id) + add_span_event('callback.received') ... +@trace_async() `@router.post`("/batch", response_model=CallbackResponse) async def handle_batch_callback( events: list[CallbackRequest], db: Session = Depends(get_db), ) -> CallbackResponse: + set_span_attribute('callback.batch_size', len(events)) + add_span_event('callback.batch.received') ...As per coding guidelines, "Use OpenTelemetry decorators from
shared/telemetry/decorators.py:@trace_async()for async functions" and "Useadd_span_event()/set_span_attribute()" for Python code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/endpoints/internal/callback.py` around lines 111 - 215, Add OpenTelemetry async tracing to the callback entrypoints by decorating both handle_callback and handle_batch_callback with `@trace_async`() from shared.telemetry.decorators and record task/subtask metadata inside the span: call set_span_attribute("task.id", request.task_id) and set_span_attribute("subtask.id", request.subtask_id) (and for batch iterate events to set attributes per-item), and emit an event with add_span_event("callback.received", {"task_id": ..., "subtask_id": ..., "event_type": ...}) at the start of each handler; ensure exceptions still propagate after logging so traces capture errors.Source: Coding guidelines
backend/app/api/endpoints/internal/chat_storage.py (1)
899-902: ⚡ Quick winInstrument the new helper with
@trace_sync().
_build_subtask_content_fields()is newly added sync code on the internal chat-storage path, but it currently bypasses the shared tracing decorator convention.As per coding guidelines, "Use OpenTelemetry decorators from
shared/telemetry/decorators.py:@trace_async()for async functions,@trace_sync()for sync functions".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/endpoints/internal/chat_storage.py` around lines 899 - 902, The helper _build_subtask_content_fields is a new synchronous function that lacks the required OpenTelemetry decorator; add the trace_sync decorator from shared.telemetry.decorators above its definition and import trace_sync (e.g., from shared.telemetry.decorators import trace_sync) so the function is instrumented consistently with project tracing conventions.Source: Coding guidelines
executor_manager/tests/executors/test_docker_executor.py (1)
635-645: ⚡ Quick winAssert the response validation step too.
This still passes if
cancel_task()stops callingraise_for_status()and only issues the POST. Since this test is meant to lock down the fallback path, please assert that the mocked response is validated as part of the success contract.Suggested tightening
- executor.requests.post.return_value = MagicMock( - raise_for_status=MagicMock(), - ) + response = MagicMock() + executor.requests.post.return_value = response @@ executor.requests.post.assert_called_once_with( f"http://{docker_executor_module.DEFAULT_DOCKER_HOST}:10000/api/tasks/cancel?task_id=6296", timeout=10, ) + response.raise_for_status.assert_called_once_with()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor_manager/tests/executors/test_docker_executor.py` around lines 635 - 645, The test currently only asserts the POST call but not that the response was validated; update the test for cancel_task to assert that the mocked response's raise_for_status() was invoked as part of the success path (e.g. call executor.requests.post.return_value.raise_for_status.assert_called_once()), so the test fails if cancel_task stops calling raise_for_status; reference the existing mocked response returned by executor.requests.post and the cancel_task function under test.backend/tests/services/chat/test_store_boundaries.py (1)
160-171: ⚡ Quick winTighten this fake to assert the full
Kindidentity.Right now any
retry.Kindquery passes. That means a regression from(namespace, name, user_id)lookup to a looser query would still stay green here, even though this path is resolving aKindboundary.As per coding guidelines,
backend/**/*.py: “A Kind resource is uniquely identified by THREE fields:namespace,name, anduser_id- always use all three fields to locate a Kind”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/chat/test_store_boundaries.py` around lines 160 - 171, The fake DB currently accepts any retry.Kind query; update the _Db.query/_Query.filter stub to assert that the filter call includes the full Kind identity (namespace, name, and user_id) matching the expected values for this test, e.g. check kwargs or positional args inside _Query.filter and raise/assert if namespace/name/user_id are missing or incorrect, then return team from _Query.first as before; reference the _Db class, query method, _Query.filter, and retry.Kind to locate where to add these assertions.Source: Coding guidelines
backend/app/services/subtask.py (1)
32-49: ⚡ Quick winInstrument the touched subtask store-boundary methods with OpenTelemetry.
These service methods were all modified in this PR, but they still lack the required
@trace_sync()decorators and span metadata for key fields liketask_id,subtask_id, and access-check outcomes. That leaves the new store boundary largely invisible in traces.As per coding guidelines,
backend/**/*.pyshould use@trace_sync()for sync functions andadd_span_event()/set_span_attribute()for span details.Also applies to: 101-109, 119-124, 142-155, 182-200, 360-379
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/subtask.py` around lines 32 - 49, The service calls to the subtask store (e.g., subtask_store.create_subtask invoked in the service function) are not instrumented with OpenTelemetry; add the `@trace_sync`() decorator to the affected synchronous service functions and within those functions call add_span_event() and/or set_span_attribute() to record key metadata such as task_id, subtask_id (returned from create_subtask), and access-check outcomes (success/failure) so the store boundary and decision points are visible in traces; update all similar spots mentioned (the blocks around lines where create_subtask and other subtask_store.* calls occur) to include these telemetry calls and ensure attribute names are consistent (e.g., "task_id", "subtask_id", "access_check") for traceability.Source: Coding guidelines
backend/app/services/knowledge/orchestrator.py (1)
305-307: ⚡ Quick winAdd tracing around this store-backed task resolution path.
This method now depends on
task_store, but the touched backend service path still has no@trace_sync()and no span attributes/events for inputs liketask_idor the resolution source (labels vs. bot chain). That makes this boundary much harder to debug once the store migration rolls out.As per coding guidelines,
backend/**/*.pyshould use@trace_sync()for sync functions andadd_span_event()/set_span_attribute()for span details.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/knowledge/orchestrator.py` around lines 305 - 307, Add tracing around the store-backed task resolution: decorate the synchronous function that contains task_store.get_by_id with `@trace_sync`(), and inside the resolution branch that calls task_store.get_by_id(db, task_id=task_id) emit span details—use set_span_attribute() to record the task_id and resolution source (e.g., "store" vs "labels" or "bot_chain") and add_span_event() to mark the resolution attempt and outcome (found/missing/invalid kind). Ensure these calls are placed around the task resolution logic where task is assigned/cleared so the trace captures inputs and the resolution path.Source: Coding guidelines
backend/app/services/knowledge/knowledge_base_qa_service.py (1)
381-396: ⚡ Quick winInstrument the new public helper.
This wrapper adds a new sync entrypoint but skips the standard tracing boundary, so calls through
get_qa_history()will bypass the repo’s normal service instrumentation.As per coding guidelines, "Use OpenTelemetry decorators from
shared/telemetry/decorators.py:@trace_async()for async functions,@trace_sync()for sync functions".📈 Minimal tracing hook
+from shared.telemetry.decorators import trace_sync + ... +@trace_sync() def get_qa_history( db: Session, start_time: datetime, end_time: datetime,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/knowledge/knowledge_base_qa_service.py` around lines 381 - 396, The new synchronous wrapper function get_qa_history bypasses the standard OpenTelemetry tracing; add the `@trace_sync`() decorator (from shared.telemetry.decorators) above the get_qa_history function and import trace_sync at the top of the file so calls routed through this helper are instrumented the same as the underlying knowledge_base_qa_service.get_qa_history.Source: Coding guidelines
backend/app/services/adapters/task_kinds/helpers.py (2)
503-508: 💤 Low valueRemove redundant local imports.
Lines 507-508 re-import
MemberStatus,ResourceMember, andResourceTypewhich are already imported at the top of the file (lines 20-21). These local imports are unnecessary.🧹 Suggested fix
def _add_group_chat_info( db: Session, tasks: List[Kind], result: Dict[str, Dict[str, Any]] ) -> None: """Add is_group_chat info to result dict using ResourceMember.""" - from app.models.resource_member import MemberStatus, ResourceMember - from app.models.share_link import ResourceType - task_ids = [t.id for t in tasks]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/adapters/task_kinds/helpers.py` around lines 503 - 508, The local imports of MemberStatus, ResourceMember, and ResourceType inside the _add_group_chat_info function are redundant; remove the lines "from app.models.resource_member import MemberStatus, ResourceMember" and "from app.models.share_link import ResourceType" from inside _add_group_chat_info and rely on the existing top-of-file imports so the function uses the already-imported MemberStatus, ResourceMember, and ResourceType symbols.
140-151: 💤 Low valueUnused
existing_subtasksparameter after store refactor.The
existing_subtasksparameter in both_create_pipeline_subtask(line 147) and_create_standard_subtask(line 210) is no longer used in the function bodies after the refactoring. The old logic that extracted executor info from existing subtasks has been removed, but the parameter remains in the signatures and is still passed fromcreate_subtasks(lines 121, 133).Consider removing this parameter from both function signatures and the corresponding arguments in the caller to reduce confusion.
Also applies to: 204-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/adapters/task_kinds/helpers.py` around lines 140 - 151, Remove the now-unused existing_subtasks parameter from the signatures of _create_pipeline_subtask and _create_standard_subtask and from every call site where create_subtasks passes it (e.g., the calls inside create_subtasks that currently pass existing_subtasks); update the functions' parameter lists and callers so they no longer accept or pass existing_subtasks, and run tests/linters to ensure no remaining references (search for existing_subtasks, _create_pipeline_subtask, _create_standard_subtask, and create_subtasks to locate all spots to update).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/endpoints/internal/chat_storage.py`:
- Around line 912-921: When updating assistant messages in update_message()
(which calls _build_subtask_content_fields() and then
subtask_store.update_fields()), avoid rebuilding result as a fresh {"value":...,
"model_info":...} that discards persisted keys; instead load the existing
message.result (or existing_result dict) and merge/overwrite only the "value"
and "model_info" fields, preserving other keys like "messages_chain" and
"loaded_skills" before passing the merged dict to subtask_store.update_fields();
apply the same merge behavior in the corresponding block at the other occurrence
(around the code that mirrors lines 1082-1086).
In `@backend/app/api/endpoints/openapi_responses.py`:
- Around line 264-269: The lookup for previous_task_id uses
task_store.get_task_by_states with owner_user_id=current_user.id, which blocks
group-member access; change it to allow member-access by falling back to a
membership-aware lookup: keep the existing owner-only call on TaskResource
resolution, but if it returns None, call task_store.get_task_by_states again for
the same task_id/states with a membership parameter (e.g.,
member_user_id=current_user.id or the existing group-membership parameter your
TaskStore supports) so group-shared tasks like resp_{group_task_id} are
resolvable; update code around existing_task and the call to get_task_by_states
to implement this two-step lookup.
In `@backend/app/api/ws/device_namespace.py`:
- Around line 113-116: The lookup for running_subtasks uses only executor_name
(in subtask_store.list_running_by_executor_name) and can return tasks belonging
to other users who share the same device_id; scope this by owner. Update the
call that sets running_subtasks to include user_id (e.g., use a
list_running_by_executor_name_and_user method or add a user_id filter) or make
executor_name globally unique by prefixing it with user_id (e.g., executor_name
= f"{user_id}:{device_id}") before calling
subtask_store.list_running_by_executor_name; ensure the same owner-scoping
convention used by set_device_online and the device key
(device:{user_id}:{device_id}) is applied so only the current user’s subtasks
are affected.
In `@backend/app/services/adapters/task_kinds/running_tasks.py`:
- Around line 5-6: Add the missing import for TaskResource so static type
checkers and typing.get_type_hints() can resolve the type: in both
running_tasks.py and task_skills_resolver.py (which already have "from
__future__ import annotations"), add "from app.models.task import TaskResource"
near the other imports, ensuring any functions or methods that annotate
parameters/returns with TaskResource (e.g., the functions that reference
TaskResource in type hints) will have the type available for static analysis and
runtime introspection.
In `@backend/app/services/device_router.py`:
- Around line 92-102: The code reloads the session-local task into local_task
via task_store.get_by_id and updates it via TaskCRD and task_store.update_json,
but later still builds the execution request from the original task and
continues when local_task is None; change the dispatch logic to use the
session-local local_task (not the original task) and immediately raise/return an
error if local_task is None after reload so you never dispatch against a
deleted/stale row; update all places that reference task when constructing the
execution payload (the dispatch/execute functions around the area formerly lines
140-153) to use local_task (and its TaskCRD if needed) and short-circuit with a
clear failure when local_task is missing.
In `@backend/app/services/device/local_provider.py`:
- Around line 492-495: The current lookup uses task_store.list_by_ids(db,
task_ids=running_task_ids) with client-controlled running_task_ids, which can
leak foreign task data; update the lookup to only return tasks owned by the
device's user: call a store method that accepts both task_ids and owner/user_id
(e.g., task_store.list_by_ids(db, task_ids=running_task_ids, user_id=user_id))
or, if such a method doesn't exist, filter the returned tasks by task.user_id ==
user_id before building running_tasks so get_status() and get_slot_usage_sync()
only include tasks owned by user_id.
In `@backend/app/services/execution/dispatcher.py`:
- Around line 870-876: Replace the boolean False with None when clearing the
timestamp-like field executor_deleted_at in both recovery paths: update the
subtask_store.update_fields call in dispatcher.py (the call that passes
executor_name/executor_namespace/executor_deleted_at) to set
executor_deleted_at=None, and make the analogous change in schedule_helper.py
where the recovery/reset logic sets executor_deleted_at; ensure the persisted
value and any update_fields/patch call use None so the field remains
timestamp-typed and semantically cleared.
In `@backend/app/services/knowledge/task_knowledge_base_service.py`:
- Around line 146-149: The legacy KB fallback must be limited to group chats: in
_is_kb_bound_to_user_group_chat() (in both task_knowledge_base_service.py and
knowledge_share_service.py) ensure you only consider tasks where
task.is_group_chat is True before inspecting task.knowledgeBaseRefs or iterating
tasks_with_kb from task_store.list_accessible_active_tasks_for_user(db,
user_id=user_id); i.e., filter tasks_with_kb to group chats or add an early
check on task.is_group_chat inside the loop so a non-group task’s legacy
knowledgeBaseRefs cannot grant implicit KB access.
In `@backend/app/services/message_forwarding_service.py`:
- Around line 97-107: Currently the code loads all subtasks via
subtask_store.list_by_task_ordered and then filters in Python when
request.subtaskIds is present, causing full scans; modify the store call to push
the subtaskIds filter into the DB query instead—either add an optional
subtask_ids (or ids) parameter to subtask_store.list_by_task_ordered and pass
request.subtaskIds, or introduce a new store method (e.g., list_by_task_and_ids
or list_by_ids_ordered) that queries by task_id + owner_user_id + subtask_ids so
the database returns only the requested subtasks and avoids loading the whole
conversation into memory. Ensure you update the service call to use that
new/extended method (reference: request.subtaskIds and
subtask_store.list_by_task_ordered).
In `@backend/app/services/subscription/service.py`:
- Around line 567-570: The workspace lookup at
task_store.get_active_workspace_by_id currently omits the owner_user_id filter
and can bind a workspace the caller doesn't own when create_or_get_workspace()
returns an existing workspace; update this call to include owner_user_id=user_id
(matching the other branches) so the lookup enforces ownership, ensuring the
workspace bound to the caller is only one they own.
In `@backend/app/services/subtask.py`:
- Around line 283-287: The bulk-delete calls
(subtask_store.delete_from_message_id and the analogous delete_to_message_id)
drop user ownership checks by forwarding only task_id and message boundaries;
restore authorization by first verifying the caller's user_id has
access/ownership for the task (e.g., call the service's existing
access/ownership check helper or validate via Task/Subtask ACL) and then pass
user_id into the store delete helper (or call a store API overload that accepts
user_id) so the store enforces per-user deletion; apply the same fix to both the
delete_from_message_id and the corresponding delete_to_message_id call sites.
In `@backend/app/stores/tasks/sqlalchemy_subtask_store.py`:
- Around line 112-129: The code is incorrectly setting
completed_at=datetime.now() for subtasks created with non-terminal statuses;
update create_assistant_subtask() and create_subtask() so they only set
Subtask.completed_at when the status is a completed/terminal status (e.g.,
SubtaskStatus.COMPLETED/FAILED), otherwise leave completed_at as None (or omit
it) when creating the Subtask instance (look for the Subtask(...) construction
where completed_at is currently assigned). Ensure any subsequent DB
inserts/updates use None for in-progress/pending subtasks so lifecycle logic
reads completion metadata correctly.
In `@backend/app/stores/tasks/sqlalchemy_task_store.py`:
- Around line 992-995: The helpers count_active_project_tasks,
get_active_project_task, and clear_project_for_owned_tasks are missing a filter
to restrict to task rows, causing non-Task resources to be counted/modified;
update each query in those methods to include a filter like TaskResource.kind ==
'Task' (or the class constant if defined, e.g., TaskResource.KIND_TASK)
alongside the existing TaskResource.project_id and TaskResource.is_active
conditions so they match list_active_project_tasks; apply the same change to the
other occurrences referenced (around the other diffs at the same functions).
- Around line 456-461: The JSON_EXTRACT predicate currently filters out rows
where $.status.status is missing (NULL); update the filter in the query that
builds on TaskResource (used by get_active_non_deleted_task / same query block)
to treat missing status as non-deleted by changing the text(...) clause to check
for NULL or non-'DELETE' (e.g. replace text("JSON_EXTRACT(json,
'$.status.status') != 'DELETE'") with a predicate like "(JSON_EXTRACT(json,
'$.status.status') IS NULL OR JSON_EXTRACT(json, '$.status.status') !=
'DELETE')" or use COALESCE/JSON_UNQUOTE to achieve the same effect) so
create_pending_task_shell() rows without status remain visible.
In `@backend/tests/stores/tasks/test_sqlalchemy_subtask_store.py`:
- Around line 21-33: The test currently checks implementation_methods <=
protocol_methods which allows the concrete class to miss required protocol
methods; change the assertion to verify that every protocol method is
implemented by asserting protocol_methods <= implementation_methods (i.e.,
ensure SubtaskStore's public methods are all present on SqlAlchemySubtaskStore),
keeping the sets constructed from vars(SqlAlchemySubtaskStore) and
vars(SubtaskStore) and the same callable and underscored-name filters.
In `@executor/app.py`:
- Around line 605-617: The current logging of request_preview (built from
openai_request and masked by mask_sensitive_data) can still expose multi-KB
prompt contents at INFO level; change the logger.info to only include metadata
plus a very short content preview (e.g., first ~50 characters) instead of using
_OPENAI_REQUEST_LOG_PREVIEW_LIMIT, and include a clear "[truncated, total size:
N chars]" suffix when truncated; specifically update the construction/usage of
request_preview and the logger.info call (referencing mask_sensitive_data,
request_preview, _OPENAI_REQUEST_LOG_PREVIEW_LIMIT, logger.info, openai_request,
task_id, subtask_id, background) so only a ~50-char snippet or purely metadata
is logged at info level.
In `@frontend/src/features/tasks/components/share/ExportSelectModal.tsx`:
- Around line 282-288: exportDocx currently drops selected rows that lack a
messageId (SelectableMessage.messageId is optional) which causes silent partial
exports; update exportDocx to detect any selectedMessages with undefined
messageId and fail fast—e.g., if selectedMessages.some(m => typeof m.messageId
!== 'number') then surface a clear error (throw or show a user-facing
notification) instead of calling taskApis.exportTaskDocx with a filtered list;
mention taskApis.exportTaskDocx, exportDocx, and the messageIds variable so
reviewers can locate and ensure the export subset always matches the UI
selection (alternatively, also add logic in the selection UI to disable/omit
rows without messageId).
---
Outside diff comments:
In `@backend/app/mcp_server/tools/subscription.py`:
- Around line 117-145: The current lookup for the Team Kind after loading the
task uses only Kind.kind/Kind.name/Kind.namespace which can return the wrong
Team for personal teams; update the query that resolves `team` (the block using
db.query(Kind).filter(...).first()) to also filter by the task owner’s user id
(use the owner id from the loaded `task` JSON / metadata) so the Kind lookup
uses the three identifying fields: namespace, name, and user_id; keep the same
extraction of modelRef (`model_ref = team.json.get("spec", {}).get("modelRef",
{})`) and model_name/namespace assignment but ensure the filter includes
`Kind.user_id == <task owner id>` to uniquely resolve the Team.
In `@backend/app/services/adapters/collaboration_strategy.py`:
- Around line 218-226: The Team lookup currently filters only by Kind.kind,
Kind.name and Kind.namespace and can return the wrong Team when multiple users
have identical namespace/name; update both call sites (the query at the shown
block and the similar one at lines ~355-363) to include the owner scope by
adding a filter for Kind.user_id == team_ref.user_id (or, better, replace both
raw queries with the existing helper
kindReader.get_by_name_and_namespace(team_ref.name, team_ref.namespace,
team_ref.user_id) to ensure all three identity fields (namespace, name, user_id)
are used when resolving a Team).
In `@backend/app/services/adapters/team_kinds.py`:
- Around line 816-824: The rename currently only updates tasks owned by the
acting user because _update_team_references_in_tasks() calls
task_store.list_regular_active_tasks(db, user_id=user_id); change this so
group-team renames enumerate all tasks in the group namespace (either by calling
the task store without the user_id filter or by querying tasks for all users in
the group/namespace) and update every task's teamRef, not just the editor's.
Update _update_team_references_in_tasks() (and the other call sites noted around
update_with_user(), lines referenced 866-868 and 1873-1891) to accept/derive the
group/namespace and fetch/update tasks across the full namespace so the rename
is applied consistently for all members. Ensure existing permission checks
(check_group_permission and Developer role) remain unchanged.
In `@backend/app/services/inbox/direct_agent_handler.py`:
- Around line 255-278: The code currently calls
task_store.list_regular_active_tasks(..., limit=50) then filters by teamRef,
which can miss older team tasks; update _find_latest_workspace_params to either
(1) extend list_regular_active_tasks to accept team filters (name/namespace) and
pass team.name and team.namespace so the DB query restricts by teamRef, or (2)
implement pagination in _find_latest_workspace_params: repeatedly call
list_regular_active_tasks with an offset/page token until a matching team_ref is
found (using task_resource.json->spec->teamRef) and only then call
task_store.get_workspace_by_ref; modify references to list_regular_active_tasks
and latest_task accordingly to ensure you don’t silently fall back when a
matching team task exists beyond the initial 50 rows.
In `@backend/tests/services/subscription/test_task_completion_handler.py`:
- Around line 45-64: The tests currently mock db.query for
Kind/TaskResource/Subtask; instead patch the stores the handler uses: replace
db.query stubs with mocks of task_store.get_task_by_states(...) and
subtask_store.list_not_executor_deleted_by_task(...) (and any
subtask_store.update_fields / task_store.update_fields calls) so the test
exercises SubscriptionTaskCompletionHandler via its collaborators; assert those
store methods are called with expected args and return the prepared task/subtask
objects rather than branching on ORM models like TaskResource/Subtask/Kind.
---
Nitpick comments:
In `@backend/app/api/endpoints/internal/callback.py`:
- Around line 111-215: Add OpenTelemetry async tracing to the callback
entrypoints by decorating both handle_callback and handle_batch_callback with
`@trace_async`() from shared.telemetry.decorators and record task/subtask metadata
inside the span: call set_span_attribute("task.id", request.task_id) and
set_span_attribute("subtask.id", request.subtask_id) (and for batch iterate
events to set attributes per-item), and emit an event with
add_span_event("callback.received", {"task_id": ..., "subtask_id": ...,
"event_type": ...}) at the start of each handler; ensure exceptions still
propagate after logging so traces capture errors.
In `@backend/app/api/endpoints/internal/chat_storage.py`:
- Around line 899-902: The helper _build_subtask_content_fields is a new
synchronous function that lacks the required OpenTelemetry decorator; add the
trace_sync decorator from shared.telemetry.decorators above its definition and
import trace_sync (e.g., from shared.telemetry.decorators import trace_sync) so
the function is instrumented consistently with project tracing conventions.
In `@backend/app/services/adapters/task_kinds/helpers.py`:
- Around line 503-508: The local imports of MemberStatus, ResourceMember, and
ResourceType inside the _add_group_chat_info function are redundant; remove the
lines "from app.models.resource_member import MemberStatus, ResourceMember" and
"from app.models.share_link import ResourceType" from inside
_add_group_chat_info and rely on the existing top-of-file imports so the
function uses the already-imported MemberStatus, ResourceMember, and
ResourceType symbols.
- Around line 140-151: Remove the now-unused existing_subtasks parameter from
the signatures of _create_pipeline_subtask and _create_standard_subtask and from
every call site where create_subtasks passes it (e.g., the calls inside
create_subtasks that currently pass existing_subtasks); update the functions'
parameter lists and callers so they no longer accept or pass existing_subtasks,
and run tests/linters to ensure no remaining references (search for
existing_subtasks, _create_pipeline_subtask, _create_standard_subtask, and
create_subtasks to locate all spots to update).
In `@backend/app/services/knowledge/knowledge_base_qa_service.py`:
- Around line 381-396: The new synchronous wrapper function get_qa_history
bypasses the standard OpenTelemetry tracing; add the `@trace_sync`() decorator
(from shared.telemetry.decorators) above the get_qa_history function and import
trace_sync at the top of the file so calls routed through this helper are
instrumented the same as the underlying
knowledge_base_qa_service.get_qa_history.
In `@backend/app/services/knowledge/orchestrator.py`:
- Around line 305-307: Add tracing around the store-backed task resolution:
decorate the synchronous function that contains task_store.get_by_id with
`@trace_sync`(), and inside the resolution branch that calls
task_store.get_by_id(db, task_id=task_id) emit span details—use
set_span_attribute() to record the task_id and resolution source (e.g., "store"
vs "labels" or "bot_chain") and add_span_event() to mark the resolution attempt
and outcome (found/missing/invalid kind). Ensure these calls are placed around
the task resolution logic where task is assigned/cleared so the trace captures
inputs and the resolution path.
In `@backend/app/services/subtask.py`:
- Around line 32-49: The service calls to the subtask store (e.g.,
subtask_store.create_subtask invoked in the service function) are not
instrumented with OpenTelemetry; add the `@trace_sync`() decorator to the affected
synchronous service functions and within those functions call add_span_event()
and/or set_span_attribute() to record key metadata such as task_id, subtask_id
(returned from create_subtask), and access-check outcomes (success/failure) so
the store boundary and decision points are visible in traces; update all similar
spots mentioned (the blocks around lines where create_subtask and other
subtask_store.* calls occur) to include these telemetry calls and ensure
attribute names are consistent (e.g., "task_id", "subtask_id", "access_check")
for traceability.
In `@backend/tests/core/test_security.py`:
- Around line 356-382: Add a negative test named like
test_non_admin_cannot_impersonate_other_user that uses _create_service_key to
create a service key owned by user A, then calls get_auth_context with that key
but passing wegent_username of a different user B; assert the call is rejected
(preferably with pytest.raises(the project's authorization/permission exception)
or, if code returns a context, assert auth_context.user.id != user B.id and
equals the key owner's id). This ensures the non-admin impersonation denial
branch for get_auth_context is covered.
In `@backend/tests/services/chat/test_store_boundaries.py`:
- Around line 160-171: The fake DB currently accepts any retry.Kind query;
update the _Db.query/_Query.filter stub to assert that the filter call includes
the full Kind identity (namespace, name, and user_id) matching the expected
values for this test, e.g. check kwargs or positional args inside _Query.filter
and raise/assert if namespace/name/user_id are missing or incorrect, then return
team from _Query.first as before; reference the _Db class, query method,
_Query.filter, and retry.Kind to locate where to add these assertions.
In `@executor_manager/tests/executors/test_docker_executor.py`:
- Around line 635-645: The test currently only asserts the POST call but not
that the response was validated; update the test for cancel_task to assert that
the mocked response's raise_for_status() was invoked as part of the success path
(e.g. call
executor.requests.post.return_value.raise_for_status.assert_called_once()), so
the test fails if cancel_task stops calling raise_for_status; reference the
existing mocked response returned by executor.requests.post and the cancel_task
function under test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5035c13b-51ca-44f4-8785-3e479664fb67
📒 Files selected for processing (111)
backend/app/api/endpoints/adapter/attachments.pybackend/app/api/endpoints/adapter/chat.pybackend/app/api/endpoints/adapter/tasks.pybackend/app/api/endpoints/internal/callback.pybackend/app/api/endpoints/internal/chat_storage.pybackend/app/api/endpoints/internal/services.pybackend/app/api/endpoints/internal/workspace_archives.pybackend/app/api/endpoints/kind/skills.pybackend/app/api/endpoints/openapi_responses.pybackend/app/api/endpoints/subtasks.pybackend/app/api/ws/chat_namespace.pybackend/app/api/ws/device_namespace.pybackend/app/core/security.pybackend/app/mcp_server/tools/silent_exit.pybackend/app/mcp_server/tools/subscription.pybackend/app/services/adapters/collaboration_strategy.pybackend/app/services/adapters/pipeline_stage.pybackend/app/services/adapters/task_kinds/converters.pybackend/app/services/adapters/task_kinds/helpers.pybackend/app/services/adapters/task_kinds/operations.pybackend/app/services/adapters/task_kinds/queries.pybackend/app/services/adapters/task_kinds/query_utils.pybackend/app/services/adapters/task_kinds/running_tasks.pybackend/app/services/adapters/task_kinds/task_skills_resolver.pybackend/app/services/adapters/team_kinds.pybackend/app/services/adapters/wework_conversation_search.pybackend/app/services/background_chat_executor.pybackend/app/services/channels/handler.pybackend/app/services/chat/access/permissions.pybackend/app/services/chat/correction/service.pybackend/app/services/chat/interactive_forms.pybackend/app/services/chat/operations/cancel.pybackend/app/services/chat/operations/retry.pybackend/app/services/chat/standalone_workspace.pybackend/app/services/chat/storage/db.pybackend/app/services/chat/storage/task_manager.pybackend/app/services/chat/trigger/lifecycle.pybackend/app/services/context/context_service.pybackend/app/services/device/local_provider.pybackend/app/services/device_monitor.pybackend/app/services/device_router.pybackend/app/services/execution/agents/image/intent_analyzer.pybackend/app/services/execution/agents/video/intent_analyzer.pybackend/app/services/execution/dispatcher.pybackend/app/services/execution/emitters/status_updating.pybackend/app/services/execution/request_builder.pybackend/app/services/execution/schedule_helper.pybackend/app/services/export/docx_generator.pybackend/app/services/inbox/direct_agent_handler.pybackend/app/services/kind.pybackend/app/services/kind_base.pybackend/app/services/kind_impl.pybackend/app/services/knowledge/knowledge_base_qa_service.pybackend/app/services/knowledge/orchestrator.pybackend/app/services/knowledge/task_knowledge_base_service.pybackend/app/services/message_forwarding_service.pybackend/app/services/object_storage/upload_grant_service.pybackend/app/services/project_service.pybackend/app/services/prompt_draft/transcript.pybackend/app/services/prompt_draft_service.pybackend/app/services/prompt_optimization.pybackend/app/services/share/knowledge_share_service.pybackend/app/services/share/share_webhook.pybackend/app/services/share/task_share_service.pybackend/app/services/shared_task.pybackend/app/services/subscription/execution.pybackend/app/services/subscription/helpers.pybackend/app/services/subscription/service.pybackend/app/services/subscription/task_completion_handler.pybackend/app/services/subscription/unified_executor.pybackend/app/services/subtask.pybackend/app/services/task_member_service.pybackend/app/services/task_status.pybackend/app/stores/tasks/__init__.pybackend/app/stores/tasks/interfaces.pybackend/app/stores/tasks/sqlalchemy_access_store.pybackend/app/stores/tasks/sqlalchemy_subtask_store.pybackend/app/stores/tasks/sqlalchemy_task_store.pybackend/app/tasks/subscription_tasks.pybackend/tests/api/endpoints/test_attachment_access.pybackend/tests/api/endpoints/test_subtasks_stream_permissions.pybackend/tests/api/test_openapi_responses.pybackend/tests/api/ws/test_chat_namespace_retry.pybackend/tests/api/ws/test_device_capabilities_state.pybackend/tests/core/test_security.pybackend/tests/services/adapters/test_task_archive_operations.pybackend/tests/services/chat/storage/test_task_manager.pybackend/tests/services/chat/test_store_boundaries.pybackend/tests/services/execution/test_dispatcher_recovery.pybackend/tests/services/execution/test_schedule_helper_recovery.pybackend/tests/services/subscription/test_subscription_service_execution_target.pybackend/tests/services/subscription/test_task_completion_handler.pybackend/tests/services/test_message_forwarding_service.pybackend/tests/services/test_project_git_workspace_service.pybackend/tests/services/test_subtask_store_integration.pybackend/tests/services/test_task_access_control.pybackend/tests/services/test_task_knowledge_base_sync.pybackend/tests/services/test_task_store_boundary_static.pybackend/tests/stores/tasks/test_sqlalchemy_subtask_store.pybackend/tests/stores/tasks/test_sqlalchemy_task_store.pychat_shell/chat_shell/main.pychat_shell/tests/test_main_request_context.pyexecutor/app.pyexecutor_manager/executors/docker/executor.pyexecutor_manager/routers/routers.pyexecutor_manager/services/task_heartbeat_manager.pyexecutor_manager/tests/executors/test_docker_executor.pyexecutor_manager/tests/routers/test_callback_logging.pyexecutor_manager/tests/routers/test_validate_image.pyfrontend/src/features/tasks/components/message/MessagesArea.tsxfrontend/src/features/tasks/components/share/ExportSelectModal.tsx
Comment on lines
+912
to
+921
| result: dict = { | ||
| "value": ( | ||
| message.content | ||
| if isinstance(message.content, str) | ||
| else json.dumps(message.content, ensure_ascii=False) | ||
| ) | ||
| } | ||
| if getattr(message, "model_info", None): | ||
| result["model_info"] = message.model_info | ||
| return {"result": result} |
Contributor
There was a problem hiding this comment.
Preserve existing assistant result fields when editing a message.
update_message() now feeds _build_subtask_content_fields() straight into subtask_store.update_fields(). For assistant rows, that helper rebuilds result as only {"value": ...} plus optional model_info, so editing a streamed turn drops persisted keys like messages_chain and loaded_skills.
🐛 Proposed fix
def _build_subtask_content_fields(
role: SubtaskRole,
message: MessageCreate | MessageUpdate,
+ *,
+ existing_result: dict[str, Any] | None = None,
) -> dict[str, Any]:
"""Build Subtask content fields from a chat message payload."""
if role == SubtaskRole.USER:
return {
"prompt": (
message.content
if isinstance(message.content, str)
else json.dumps(message.content, ensure_ascii=False)
)
}
- result: dict = {
- "value": (
- message.content
- if isinstance(message.content, str)
- else json.dumps(message.content, ensure_ascii=False)
- )
- }
+ result: dict[str, Any] = dict(existing_result or {})
+ result["value"] = (
+ message.content
+ if isinstance(message.content, str)
+ else json.dumps(message.content, ensure_ascii=False)
+ )
if getattr(message, "model_info", None):
result["model_info"] = message.model_info
return {"result": result}
@@
subtask_store.update_fields(
db,
subtask=subtask,
- **_build_subtask_content_fields(subtask.role, update),
+ **_build_subtask_content_fields(
+ subtask.role,
+ update,
+ existing_result=subtask.result if isinstance(subtask.result, dict) else None,
+ ),
)Also applies to: 1082-1086
🧰 Tools
🪛 ast-grep (0.43.0)
[info] 915-915: use jsonify instead of json.dumps for JSON output
Context: json.dumps(message.content, ensure_ascii=False)
Note: Security best practice.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/endpoints/internal/chat_storage.py` around lines 912 - 921,
When updating assistant messages in update_message() (which calls
_build_subtask_content_fields() and then subtask_store.update_fields()), avoid
rebuilding result as a fresh {"value":..., "model_info":...} that discards
persisted keys; instead load the existing message.result (or existing_result
dict) and merge/overwrite only the "value" and "model_info" fields, preserving
other keys like "messages_chain" and "loaded_skills" before passing the merged
dict to subtask_store.update_fields(); apply the same merge behavior in the
corresponding block at the other occurrence (around the code that mirrors lines
1082-1086).
Comment on lines
+264
to
269
| existing_task = task_store.get_task_by_states( | ||
| db, | ||
| task_id=previous_task_id, | ||
| states=[TaskResource.STATE_ACTIVE], | ||
| owner_user_id=current_user.id, | ||
| ) |
Contributor
There was a problem hiding this comment.
Allow member-access tasks in previous_response_id resolution.
This lookup is owner-only, but the new cancel path already treats group membership as sufficient access. A member following up on resp_{group_task_id} will now 404 here even though the same task is otherwise accessible, so shared/group conversations break on the create path.
Suggested fix
existing_task = task_store.get_task_by_states(
db,
task_id=previous_task_id,
states=[TaskResource.STATE_ACTIVE],
owner_user_id=current_user.id,
)
+ if not existing_task:
+ member_task = task_store.get_task_by_states(
+ db,
+ task_id=previous_task_id,
+ states=[TaskResource.STATE_ACTIVE],
+ )
+ if member_task and task_access_store.is_member(
+ db,
+ task_id=previous_task_id,
+ user_id=current_user.id,
+ ):
+ existing_task = member_task
if not existing_task:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail=f"Previous response '{request_body.previous_response_id}' not found",
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/endpoints/openapi_responses.py` around lines 264 - 269, The
lookup for previous_task_id uses task_store.get_task_by_states with
owner_user_id=current_user.id, which blocks group-member access; change it to
allow member-access by falling back to a membership-aware lookup: keep the
existing owner-only call on TaskResource resolution, but if it returns None,
call task_store.get_task_by_states again for the same task_id/states with a
membership parameter (e.g., member_user_id=current_user.id or the existing
group-membership parameter your TaskStore supports) so group-shared tasks like
resp_{group_task_id} are resolvable; update code around existing_task and the
call to get_task_by_states to implement this two-step lookup.
Comment on lines
+113
to
116
| running_subtasks = subtask_store.list_running_by_executor_name( | ||
| db, | ||
| executor_name=executor_name, | ||
| ) |
Contributor
There was a problem hiding this comment.
Scope the disconnect lookup by owner, not just executor_name.
Line 113 derives executor_name from device_id alone, but this namespace treats devices as user-scoped everywhere else (set_device_online(user_id, device_id, ...), device:{user_id}:{device_id}). If two users register the same device_id, this store query can fetch the other user's running subtasks and the loop below will mark those subtasks and parent tasks as failed. The lookup needs user_id scoping as well, or the executor name must be made globally unique per owner before using it as a failure key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/ws/device_namespace.py` around lines 113 - 116, The lookup
for running_subtasks uses only executor_name (in
subtask_store.list_running_by_executor_name) and can return tasks belonging to
other users who share the same device_id; scope this by owner. Update the call
that sets running_subtasks to include user_id (e.g., use a
list_running_by_executor_name_and_user method or add a user_id filter) or make
executor_name globally unique by prefixing it with user_id (e.g., executor_name
= f"{user_id}:{device_id}") before calling
subtask_store.list_running_by_executor_name; ensure the same owner-scoping
convention used by set_device_online and the device key
(device:{user_id}:{device_id}) is applied so only the current user’s subtasks
are affected.
Comment on lines
+5
to
+6
| from __future__ import annotations | ||
|
|
Contributor
There was a problem hiding this comment.
Missing TaskResource import in running_tasks.py and task_skills_resolver.py.
Both files add from __future__ import annotations to enable postponed type evaluation, but use TaskResource in function type hints without importing it from app.models.task. While postponed evaluation prevents a runtime NameError at import time, this breaks static type checkers (mypy/pyright) and runtime introspection via typing.get_type_hints(). Add the missing import to both files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/adapters/task_kinds/running_tasks.py` around lines 5 -
6, Add the missing import for TaskResource so static type checkers and
typing.get_type_hints() can resolve the type: in both running_tasks.py and
task_skills_resolver.py (which already have "from __future__ import
annotations"), add "from app.models.task import TaskResource" near the other
imports, ensuring any functions or methods that annotate parameters/returns with
TaskResource (e.g., the functions that reference TaskResource in type hints)
will have the type available for static analysis and runtime introspection.
Comment on lines
+92
to
+102
| local_task = task_store.get_by_id(db, task_id=task.id) | ||
| if local_task and local_task.kind == "Task": | ||
| from app.schemas.kind import Task as TaskCRD | ||
|
|
||
| task_crd = TaskCRD.model_validate(local_task.json) | ||
| if not task_crd.spec.device_id or task_crd.spec.device_id != device_id: | ||
| task_crd.spec.device_id = device_id | ||
| local_task.json = task_crd.model_dump(mode="json") | ||
| db.add(local_task) | ||
| task_store.update_json( | ||
| db, | ||
| task=local_task, | ||
| payload=task_crd.model_dump(mode="json"), |
Contributor
There was a problem hiding this comment.
Use the session-local task for dispatch and fail fast if it disappeared.
local_task is reloaded on Line 92 to avoid stale/cross-session ORM state, but Lines 140-143 still build the execution request from the original task object and the function keeps going when local_task is None. That can dispatch against a deleted or stale task record instead of the row you just validated and updated.
Suggested fix
local_task = task_store.get_by_id(db, task_id=task.id)
+ if not local_task:
+ raise HTTPException(status_code=404, detail="Task not found")
+
if local_task and local_task.kind == "Task":
from app.schemas.kind import Task as TaskCRD
task_crd = TaskCRD.model_validate(local_task.json)
@@
request = builder.build(
subtask=local_subtask,
- task=task,
+ task=local_task,
user=user,
team=team,
message=(
messageAlso applies to: 140-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/device_router.py` around lines 92 - 102, The code
reloads the session-local task into local_task via task_store.get_by_id and
updates it via TaskCRD and task_store.update_json, but later still builds the
execution request from the original task and continues when local_task is None;
change the dispatch logic to use the session-local local_task (not the original
task) and immediately raise/return an error if local_task is None after reload
so you never dispatch against a deleted/stale row; update all places that
reference task when constructing the execution payload (the dispatch/execute
functions around the area formerly lines 140-153) to use local_task (and its
TaskCRD if needed) and short-circuit with a clear failure when local_task is
missing.
Comment on lines
+456
to
+461
| query = db.query(TaskResource).filter( | ||
| TaskResource.id == task_id, | ||
| TaskResource.kind == "Task", | ||
| TaskResource.is_active.in_(TaskResource.is_active_query()), | ||
| text("JSON_EXTRACT(json, '$.status.status') != 'DELETE'"), | ||
| ) |
Contributor
There was a problem hiding this comment.
Treat missing $.status.status as non-deleted.
create_pending_task_shell() stores json={"kind": "Task"} with no status.status, so this predicate evaluates to SQL NULL and filters the row out. Right now an active shell can be created successfully and then immediately become invisible to get_active_non_deleted_task().
Suggested fix
query = db.query(TaskResource).filter(
TaskResource.id == task_id,
TaskResource.kind == "Task",
TaskResource.is_active.in_(TaskResource.is_active_query()),
- text("JSON_EXTRACT(json, '$.status.status') != 'DELETE'"),
+ text(
+ "COALESCE(JSON_UNQUOTE(JSON_EXTRACT(json, '$.status.status')), '') != 'DELETE'"
+ ),
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/stores/tasks/sqlalchemy_task_store.py` around lines 456 - 461,
The JSON_EXTRACT predicate currently filters out rows where $.status.status is
missing (NULL); update the filter in the query that builds on TaskResource (used
by get_active_non_deleted_task / same query block) to treat missing status as
non-deleted by changing the text(...) clause to check for NULL or non-'DELETE'
(e.g. replace text("JSON_EXTRACT(json, '$.status.status') != 'DELETE'") with a
predicate like "(JSON_EXTRACT(json, '$.status.status') IS NULL OR
JSON_EXTRACT(json, '$.status.status') != 'DELETE')" or use COALESCE/JSON_UNQUOTE
to achieve the same effect) so create_pending_task_shell() rows without status
remain visible.
Comment on lines
+992
to
+995
| query = db.query(func.count(TaskResource.id)).filter( | ||
| TaskResource.project_id == project_id, | ||
| TaskResource.is_active == TaskResource.STATE_ACTIVE, | ||
| ) |
Contributor
There was a problem hiding this comment.
Keep task-specific project helpers scoped to kind == 'Task'.
This store writes multiple resource kinds into tasks, but count_active_project_tasks(), get_active_project_task(), and clear_project_for_owned_tasks() don't add a TaskResource.kind == 'Task' filter. That makes these task-only helpers inconsistent with list_active_project_tasks() and can count or mutate non-task rows if a workspace/placeholder/custom resource shares the same project_id.
Also applies to: 1028-1032, 1130-1133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/stores/tasks/sqlalchemy_task_store.py` around lines 992 - 995,
The helpers count_active_project_tasks, get_active_project_task, and
clear_project_for_owned_tasks are missing a filter to restrict to task rows,
causing non-Task resources to be counted/modified; update each query in those
methods to include a filter like TaskResource.kind == 'Task' (or the class
constant if defined, e.g., TaskResource.KIND_TASK) alongside the existing
TaskResource.project_id and TaskResource.is_active conditions so they match
list_active_project_tasks; apply the same change to the other occurrences
referenced (around the other diffs at the same functions).
Comment on lines
+21
to
+33
| def test_subtask_store_protocol_declares_all_public_implementation_methods() -> None: | ||
| implementation_methods = { | ||
| name | ||
| for name, value in vars(SqlAlchemySubtaskStore).items() | ||
| if callable(value) and not name.startswith("_") | ||
| } | ||
| protocol_methods = { | ||
| name | ||
| for name, value in vars(SubtaskStore).items() | ||
| if callable(value) and not name.startswith("_") | ||
| } | ||
|
|
||
| assert implementation_methods <= protocol_methods |
Contributor
There was a problem hiding this comment.
The protocol coverage assertion is backwards.
implementation_methods <= protocol_methods passes when SqlAlchemySubtaskStore is missing required protocol methods. To verify implementation coverage, this needs to assert the protocol method set is contained in the implementation set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/stores/tasks/test_sqlalchemy_subtask_store.py` around lines 21
- 33, The test currently checks implementation_methods <= protocol_methods which
allows the concrete class to miss required protocol methods; change the
assertion to verify that every protocol method is implemented by asserting
protocol_methods <= implementation_methods (i.e., ensure SubtaskStore's public
methods are all present on SqlAlchemySubtaskStore), keeping the sets constructed
from vars(SqlAlchemySubtaskStore) and vars(SubtaskStore) and the same callable
and underscored-name filters.
Comment on lines
+605
to
617
| request_preview = json.dumps( | ||
| mask_sensitive_data(openai_request), ensure_ascii=False, default=str | ||
| ) | ||
| if len(request_preview) > _OPENAI_REQUEST_LOG_PREVIEW_LIMIT: | ||
| request_preview = ( | ||
| request_preview[:_OPENAI_REQUEST_LOG_PREVIEW_LIMIT] | ||
| + f"... [truncated, total size: {len(request_preview)} chars]" | ||
| ) | ||
|
|
||
| logger.info( | ||
| f"[v1/responses] Received OpenAI request: task_id={task_id}, " | ||
| f"subtask_id={subtask_id}, background={background}, request={openai_request}" | ||
| f"subtask_id={subtask_id}, background={background}, request={request_preview}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Avoid logging multi-KB request previews at info level.
mask_sensitive_data() redacts secrets, but it does not remove prompt/input/instructions content. This still writes up to 4 KB of user request data to normal executor logs on every /v1/responses call, which is much larger than the repo’s established short-preview pattern and can leak private task content. Prefer metadata-only logging, or cap any preview to a very small slice.
Based on learnings, only short (~50-character) query previews are treated as intentional in this repo.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/app.py` around lines 605 - 617, The current logging of
request_preview (built from openai_request and masked by mask_sensitive_data)
can still expose multi-KB prompt contents at INFO level; change the logger.info
to only include metadata plus a very short content preview (e.g., first ~50
characters) instead of using _OPENAI_REQUEST_LOG_PREVIEW_LIMIT, and include a
clear "[truncated, total size: N chars]" suffix when truncated; specifically
update the construction/usage of request_preview and the logger.info call
(referencing mask_sensitive_data, request_preview,
_OPENAI_REQUEST_LOG_PREVIEW_LIMIT, logger.info, openai_request, task_id,
subtask_id, background) so only a ~50-char snippet or purely metadata is logged
at info level.
Source: Learnings
Comment on lines
282
to
288
| const exportDocx = async (selectedMessages: SelectableMessage[]) => { | ||
| // Extract numeric message IDs for the API call | ||
| const messageIds = selectedMessages | ||
| .map(msg => { | ||
| const id = typeof msg.id === 'string' ? parseInt(msg.id, 10) : msg.id | ||
| return isNaN(id) ? null : id | ||
| }) | ||
| .filter((id): id is number => id !== null) | ||
| .map(msg => msg.messageId) | ||
| .filter((id): id is number => typeof id === 'number') | ||
|
|
||
| const blob = await taskApis.exportTaskDocx(taskId, messageIds) |
Contributor
There was a problem hiding this comment.
Don't silently drop selected messages from DOCX export.
messageId is optional on SelectableMessage, but this path just filters out every selected row that doesn't have one. That means the user can select N messages in the modal and still get a partial DOCX with no warning. Fail fast here or disable those rows up front so the exported subset always matches the UI selection.
Suggested fix
const messageIds = selectedMessages
.map(msg => msg.messageId)
.filter((id): id is number => typeof id === 'number')
+
+ if (messageIds.length !== selectedMessages.length) {
+ throw new Error('Some selected messages cannot be exported to DOCX yet')
+ }
const blob = await taskApis.exportTaskDocx(taskId, messageIds)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/features/tasks/components/share/ExportSelectModal.tsx` around
lines 282 - 288, exportDocx currently drops selected rows that lack a messageId
(SelectableMessage.messageId is optional) which causes silent partial exports;
update exportDocx to detect any selectedMessages with undefined messageId and
fail fast—e.g., if selectedMessages.some(m => typeof m.messageId !== 'number')
then surface a clear error (throw or show a user-facing notification) instead of
calling taskApis.exportTaskDocx with a filtered list; mention
taskApis.exportTaskDocx, exportDocx, and the messageIds variable so reviewers
can locate and ensure the export subset always matches the UI selection
(alternatively, also add logic in the selection UI to disable/omit rows without
messageId).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executor_manager/tests/executors/test_docker_executor.py (1)
642-675:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMock the port lookup that
cancel_task()actually uses.This test currently stubs
get_container_ports(), but the cancel path is still resolving the host port viaexecutor._get_container_port(...), so the mock never participates and the assertion fails in CI. The nearby test above already patches the right seam.Suggested fix
- `@patch`("executor_manager.executors.docker.executor.get_container_ports") `@patch`("executor_manager.executors.docker.executor.get_running_task_details") def test_cancel_task_uses_container_details_when_task_ids_empty( - self, mock_get_running_task_details, mock_get_container_ports, executor + self, mock_get_running_task_details, executor ): """Cancel should use matching container details even when task_ids is empty.""" mock_get_running_task_details.return_value = { "status": "success", "task_ids": [], @@ - mock_get_container_ports.return_value = { - "status": "success", - "ports": [{"container_port": 10000, "host_port": 10000}], - } executor.requests.post.return_value = MagicMock( raise_for_status=MagicMock(), ) - result = executor.cancel_task(6296) + with patch.object(executor, "_get_container_port", return_value=(10000, None)): + result = executor.cancel_task(6296)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor_manager/tests/executors/test_docker_executor.py` around lines 642 - 675, The test patches get_container_ports but cancel_task actually calls executor._get_container_port, so update the test to patch/mock executor._get_container_port to return the host port (e.g. 10000) so cancel_task will use that value; keep the existing mock_get_running_task_details (for cancel_task) and ensure executor.requests.post.assert_called_once_with(...) still expects the f"http://{docker_executor_module.DEFAULT_DOCKER_HOST}:10000/..." URL — in short, replace or add a patch for executor._get_container_port to return the correct port so the assertion succeeds.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/endpoints/internal/workspace_archives.py`:
- Around line 108-117: The endpoint currently creates a synthetic ORM Subtask
instance (Subtask) to call archive_service.archive_workspace, violating the
task/subtask boundary; remove the Subtask creation from workspace_archives.py
and instead call a service API that takes primitive metadata (e.g., task_id,
runtime_type, executor_name, executor_namespace) or add a sandbox-specific
service method (e.g., archive_workspace_sandbox) that accepts task_id and
runtime_type; update archive_service.archive_workspace (or add the new
archive_workspace_sandbox) so it consumes task_id/runtime_type and performs any
subtask ORM lookups/creation inside the service, then adjust the endpoint call
to pass those primitives to the service.
---
Outside diff comments:
In `@executor_manager/tests/executors/test_docker_executor.py`:
- Around line 642-675: The test patches get_container_ports but cancel_task
actually calls executor._get_container_port, so update the test to patch/mock
executor._get_container_port to return the host port (e.g. 10000) so cancel_task
will use that value; keep the existing mock_get_running_task_details (for
cancel_task) and ensure executor.requests.post.assert_called_once_with(...)
still expects the
f"http://{docker_executor_module.DEFAULT_DOCKER_HOST}:10000/..." URL — in short,
replace or add a patch for executor._get_container_port to return the correct
port so the assertion succeeds.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3562ec2a-d27d-4717-af20-afecb8649621
📒 Files selected for processing (12)
backend/app/api/endpoints/internal/chat_storage.pybackend/app/api/endpoints/internal/workspace_archives.pybackend/app/api/endpoints/subtasks.pybackend/app/api/ws/chat_namespace.pybackend/app/services/adapters/task_kinds/converters.pybackend/app/services/execution/dispatcher.pybackend/app/services/execution/request_builder.pybackend/app/services/knowledge/orchestrator.pyexecutor/app.pyexecutor_manager/executors/docker/executor.pyexecutor_manager/routers/routers.pyexecutor_manager/tests/executors/test_docker_executor.py
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/app/services/execution/request_builder.py
- backend/app/services/execution/dispatcher.py
- backend/app/services/adapters/task_kinds/converters.py
- executor/app.py
- backend/app/api/endpoints/internal/chat_storage.py
- executor_manager/executors/docker/executor.py
- executor_manager/routers/routers.py
- backend/app/api/ws/chat_namespace.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executor_manager/tests/executors/test_docker_executor.py (1)
642-675:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMock the port lookup that
cancel_task()actually uses.This test currently stubs
get_container_ports(), but the cancel path is still resolving the host port viaexecutor._get_container_port(...), so the mock never participates and the assertion fails in CI. The nearby test above already patches the right seam.Suggested fix
- `@patch`("executor_manager.executors.docker.executor.get_container_ports") `@patch`("executor_manager.executors.docker.executor.get_running_task_details") def test_cancel_task_uses_container_details_when_task_ids_empty( - self, mock_get_running_task_details, mock_get_container_ports, executor + self, mock_get_running_task_details, executor ): """Cancel should use matching container details even when task_ids is empty.""" mock_get_running_task_details.return_value = { "status": "success", "task_ids": [], @@ - mock_get_container_ports.return_value = { - "status": "success", - "ports": [{"container_port": 10000, "host_port": 10000}], - } executor.requests.post.return_value = MagicMock( raise_for_status=MagicMock(), ) - result = executor.cancel_task(6296) + with patch.object(executor, "_get_container_port", return_value=(10000, None)): + result = executor.cancel_task(6296)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor_manager/tests/executors/test_docker_executor.py` around lines 642 - 675, The test patches get_container_ports but cancel_task actually calls executor._get_container_port, so update the test to patch/mock executor._get_container_port to return the host port (e.g. 10000) so cancel_task will use that value; keep the existing mock_get_running_task_details (for cancel_task) and ensure executor.requests.post.assert_called_once_with(...) still expects the f"http://{docker_executor_module.DEFAULT_DOCKER_HOST}:10000/..." URL — in short, replace or add a patch for executor._get_container_port to return the correct port so the assertion succeeds.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/endpoints/internal/workspace_archives.py`:
- Around line 108-117: The endpoint currently creates a synthetic ORM Subtask
instance (Subtask) to call archive_service.archive_workspace, violating the
task/subtask boundary; remove the Subtask creation from workspace_archives.py
and instead call a service API that takes primitive metadata (e.g., task_id,
runtime_type, executor_name, executor_namespace) or add a sandbox-specific
service method (e.g., archive_workspace_sandbox) that accepts task_id and
runtime_type; update archive_service.archive_workspace (or add the new
archive_workspace_sandbox) so it consumes task_id/runtime_type and performs any
subtask ORM lookups/creation inside the service, then adjust the endpoint call
to pass those primitives to the service.
---
Outside diff comments:
In `@executor_manager/tests/executors/test_docker_executor.py`:
- Around line 642-675: The test patches get_container_ports but cancel_task
actually calls executor._get_container_port, so update the test to patch/mock
executor._get_container_port to return the host port (e.g. 10000) so cancel_task
will use that value; keep the existing mock_get_running_task_details (for
cancel_task) and ensure executor.requests.post.assert_called_once_with(...)
still expects the
f"http://{docker_executor_module.DEFAULT_DOCKER_HOST}:10000/..." URL — in short,
replace or add a patch for executor._get_container_port to return the correct
port so the assertion succeeds.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3562ec2a-d27d-4717-af20-afecb8649621
📒 Files selected for processing (12)
backend/app/api/endpoints/internal/chat_storage.pybackend/app/api/endpoints/internal/workspace_archives.pybackend/app/api/endpoints/subtasks.pybackend/app/api/ws/chat_namespace.pybackend/app/services/adapters/task_kinds/converters.pybackend/app/services/execution/dispatcher.pybackend/app/services/execution/request_builder.pybackend/app/services/knowledge/orchestrator.pyexecutor/app.pyexecutor_manager/executors/docker/executor.pyexecutor_manager/routers/routers.pyexecutor_manager/tests/executors/test_docker_executor.py
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/app/services/execution/request_builder.py
- backend/app/services/execution/dispatcher.py
- backend/app/services/adapters/task_kinds/converters.py
- executor/app.py
- backend/app/api/endpoints/internal/chat_storage.py
- executor_manager/executors/docker/executor.py
- executor_manager/routers/routers.py
- backend/app/api/ws/chat_namespace.py
🛑 Comments failed to post (1)
backend/app/api/endpoints/internal/workspace_archives.py (1)
108-117:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t instantiate
Subtaskin the endpoint layer.Line 108 manufactures a dummy ORM object just to satisfy
archive_service.archive_workspace(). That breaks the task/subtask store boundary this PR is introducing, and the static boundary test is already failing on it. The fix belongs at the service boundary: add a sandbox-specific archive API or let the service accept task/runtime metadata instead of a syntheticSubtask.Directional fix
-from app.models.subtask import Subtask @@ - subtask = Subtask(task_id=task_id) - - archive_info = await archive_service.archive_workspace( + archive_info = await archive_service.archive_sandbox_workspace( db=db, - subtask=subtask, task=task, executor_name=request.executor_name, executor_namespace=request.executor_namespace, - runtime_type="sandbox", )If you want to keep a single service method, change that method to accept
task_id/runtime_typedirectly instead of requiring an ORMSubtask.🧰 Tools
🪛 GitHub Actions: Tests / 7_Test Backend.txt
[error] 108-108: AssertionError from store boundary static check: unexpected ORM construction detected: 'subtask = Subtask(task_id=task_id)'. (tests/services/test_task_store_boundary_static.py::test_task_subtask_orm_construction_stays_inside_stores)
🪛 GitHub Actions: Tests / Test Backend
[error] 108-108: Assertion failed in tests/services/test_task_store_boundary_static.py::test_task_subtask_orm_construction_stays_inside_stores: detected forbidden ORM construction. Offending line: 'subtask = Subtask(task_id=task_id)'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/endpoints/internal/workspace_archives.py` around lines 108 - 117, The endpoint currently creates a synthetic ORM Subtask instance (Subtask) to call archive_service.archive_workspace, violating the task/subtask boundary; remove the Subtask creation from workspace_archives.py and instead call a service API that takes primitive metadata (e.g., task_id, runtime_type, executor_name, executor_namespace) or add a sandbox-specific service method (e.g., archive_workspace_sandbox) that accepts task_id and runtime_type; update archive_service.archive_workspace (or add the new archive_workspace_sandbox) so it consumes task_id/runtime_type and performs any subtask ORM lookups/creation inside the service, then adjust the endpoint call to pass those primitives to the service.Source: Pipeline failures
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/turn_file_changes.py (1)
124-162:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
taskvariable assignment causes NameError.The refactoring calls
task_store.get_active_taskinline in the conditional (lines 130–134) but never assigns the result to a variable. Line 152 referencestask.jsonfor device_id validation, buttaskis undefined. This will crash at runtime withNameError: name 'task' is not definedwhen the method reaches the device validation block.🐛 Proposed fix to assign task and restructure validation
def _load_summary( self, db: Session, user_id: int, subtask_id: int, ) -> tuple[Subtask, TurnFileChangesSummary]: subtask = subtask_store.get_by_id_and_role( db, subtask_id=subtask_id, role=SubtaskRole.ASSISTANT, owner_user_id=user_id, ) - if subtask is None or not task_store.get_active_task( + task = None + if subtask is not None: + task = task_store.get_active_task( + db, + task_id=subtask.task_id, + owner_user_id=user_id, + ) + if subtask is None or task is None: - db, - task_id=subtask.task_id, - owner_user_id=user_id, - ): raise _error( 404, "TURN_FILE_CHANGES_NOT_FOUND", "Assistant message file changes were not found", ) result = subtask.result if isinstance(subtask.result, dict) else {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/turn_file_changes.py` around lines 124 - 162, The code calls task_store.get_active_task inline and never assigns it, then later references task.json leading to a NameError; fix by assigning the result of task_store.get_active_task to a variable (e.g., task = task_store.get_active_task(...)) when you check existence, use that task variable for the device_id/spec lookup, and keep the existing TurnFileChangesSummary validation and expected_device_id vs summary.device_id comparison logic (ensure you still handle None and dict checks for task.json and task_spec).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/app/services/turn_file_changes.py`:
- Around line 124-162: The code calls task_store.get_active_task inline and
never assigns it, then later references task.json leading to a NameError; fix by
assigning the result of task_store.get_active_task to a variable (e.g., task =
task_store.get_active_task(...)) when you check existence, use that task
variable for the device_id/spec lookup, and keep the existing
TurnFileChangesSummary validation and expected_device_id vs summary.device_id
comparison logic (ensure you still handle None and dict checks for task.json and
task_spec).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c127f47e-9da7-4561-bb03-1dfff0ee8885
📒 Files selected for processing (8)
backend/app/api/endpoints/internal/workspace_archives.pybackend/app/api/ws/chat_namespace.pybackend/app/services/adapters/executor_job.pybackend/app/services/turn_file_changes.pybackend/app/services/workspace_archive/archive_service.pybackend/tests/services/adapters/test_task_kinds_delete_runtime.pybackend/tests/services/test_preserve_executor.pyexecutor_manager/tests/executors/test_docker_executor.py
💤 Files with no reviewable changes (3)
- backend/app/services/adapters/executor_job.py
- backend/app/services/workspace_archive/archive_service.py
- backend/app/api/endpoints/internal/workspace_archives.py
✅ Files skipped from review due to trivial changes (1)
- backend/tests/services/adapters/test_task_kinds_delete_runtime.py
🚧 Files skipped from review as they are similar to previous changes (2)
- executor_manager/tests/executors/test_docker_executor.py
- backend/app/api/ws/chat_namespace.py
feifei325
changed the title
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
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 by CodeRabbit
Bug Fixes
Improvements
Tests