Repository navigation
Conversation
Contributor
📝 WalkthroughWalkthroughA comprehensive IM bot redesign splits the previous single-purpose bot into two explicit types ( ChangesIM Bot Skill Redesign Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Sequence DiagramsequenceDiagram
participant User
participant IMBot
participant IMControlSkill
participant MCPTool
participant IMControlService
participant Redis
participant SessionAPI
User->>IMBot: "show my current session"
IMBot->>IMControlSkill: route to im-control intent
IMControlSkill->>MCPTool: call get_current_state(token)
MCPTool->>SessionAPI: fetch session using token claims
SessionAPI-->>MCPTool: IMPrivateSession with bot_purpose
MCPTool->>IMControlService: get_current_state(session, bot_purpose)
IMControlService-->>MCPTool: {status: success, state: {...}}
MCPTool-->>IMControlSkill: tool result message
IMControlSkill-->>User: "You are bound to task X in wework_local mode"
Note over User,Redis: Confirmation scenario for wework_local clear
User->>IMBot: "clear my current session"
IMBot->>IMControlSkill: route to clear intent
IMControlSkill->>MCPTool: call clear_current_session(token)
MCPTool->>IMControlService: clear_current_session(session, bot_purpose=wework_local)
alt Active runtime task exists
IMControlService->>Redis: store pending action (short TTL)
IMControlService-->>MCPTool: {status: needs_confirmation, action_id: abc123}
MCPTool-->>IMControlSkill: needs confirmation
IMControlSkill-->>User: "Clearing will disconnect task X. Confirm?"
User->>IMControlSkill: "yes, confirm"
IMControlSkill->>MCPTool: confirm_pending_action(token, action_id=abc123)
MCPTool->>Redis: validate pending action
Redis-->>MCPTool: valid
MCPTool->>IMControlService: confirm_pending_action(session, action_id)
IMControlService->>IMControlService: clear task binding
IMControlService-->>MCPTool: {status: success, state: {...}}
else No active task
IMControlService->>IMControlService: clear immediately
IMControlService-->>MCPTool: {status: success, state: {...}}
end
MCPTool-->>IMControlSkill: tool result
IMControlSkill-->>User: "Session cleared"
Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
qdaxb
marked this pull request as ready for review
June 24, 2026 02:44
![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: 6
🧹 Nitpick comments (9)
backend/tests/services/im/test_control_service.py (1)
62-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for cancel-without-
action_id.Please add a case that cancels a pending action without passing
action_id, then verifies the old ID cannot be confirmed anymore. This protects the pending-action revocation contract.🤖 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/im/test_control_service.py` around lines 62 - 89, Add a new regression test function that verifies the pending-action revocation contract when canceling without an action_id. Create a test that: first calls clear_current_session (like the existing test_confirm_pending_clear_removes_wework_task_binding), then invokes another operation that cancels the pending action without passing the action_id parameter, and finally attempts to confirm the pending action using the old action_id and asserts that this confirmation fails or returns an error status instead of success. This ensures that once a pending action is revoked through the cancel-without-action_id pathway, the old action_id can no longer be used for confirmation.backend/app/api/endpoints/im_sessions.py (2)
39-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tracing to the updated async endpoint.
Line 39 updates an async endpoint without
@trace_async.As per coding guidelines,
**/*.py: “Use@trace_asyncor@trace_syncdecorators from shared.telemetry.decorators for tracing async and sync functions respectively.”🤖 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/im_sessions.py` around lines 39 - 46, The async function list_private_sessions is missing the required `@trace_async` decorator. Add the `@trace_async` decorator imported from shared.telemetry.decorators above the function definition of list_private_sessions to enable proper tracing for this async endpoint, following the coding guidelines that require all async functions to be decorated with either `@trace_async` or `@trace_sync`.Source: Coding guidelines
49-55: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid repeated bot-purpose lookups for sessions on the same channel.
Lines 49-55 do one lookup per session. If a user has multiple sessions on one channel, this repeats the same DB query and scales poorly.
♻️ Suggested refactor
sessions = await im_session_service.list_user_sessions(db, user_id=current_user.id) items: list[IMPrivateSessionOut] = [] + purpose_by_channel_id: dict[int, str] = {} for session in sessions: - session_bot_purpose = await im_session_service.get_session_bot_purpose( - db, - session, - ) + session_bot_purpose = purpose_by_channel_id.get(session.channel_id) + if session_bot_purpose is None: + session_bot_purpose = await im_session_service.get_session_bot_purpose( + db, + session, + ) + purpose_by_channel_id[session.channel_id] = session_bot_purpose if bot_purpose and session_bot_purpose != bot_purpose: continue🤖 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/im_sessions.py` around lines 49 - 55, The code in the loop iterating through sessions calls get_session_bot_purpose for each session individually, which causes redundant database queries when multiple sessions belong to the same channel. Refactor the code to first collect all unique channels from the sessions list, perform a single batch lookup or mapping operation to get bot purposes for all unique channels, then use this channel-to-bot-purpose mapping within the loop instead of calling get_session_bot_purpose repeatedly. This will eliminate duplicate queries and improve performance as the number of sessions grows.backend/tests/services/auth/test_task_token_im_context.py (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit
-> Nonereturn annotation to this test function.As per coding guidelines,
**/*.py: "Use type hints for all function parameters and return values."Proposed patch
-def test_task_token_round_trips_im_context(): +def test_task_token_round_trips_im_context() -> None:🤖 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/auth/test_task_token_im_context.py` at line 8, Add an explicit `-> None` return type annotation to the test_task_token_round_trips_im_context function definition to comply with the coding guideline requiring type hints for all function parameters and return values. Simply add `-> None` after the function parameters and before the colon.Source: Coding guidelines
backend/tests/mcp_server/test_server_routes.py (1)
134-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit
-> Nonereturn annotations to the new test functions.These new test defs are missing return type hints required by the repo Python guideline.
As per coding guidelines,
**/*.py: "Use type hints for all function parameters and return values."Proposed patch
-def test_im_control_mcp_root_returns_metadata_json(): +def test_im_control_mcp_root_returns_metadata_json() -> None: @@ -def test_get_mcp_im_control_config_uses_sse_endpoint(): +def test_get_mcp_im_control_config_uses_sse_endpoint() -> None: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/tests/mcp_server/test_server_routes.py` around lines 134 - 150, The test function test_im_control_mcp_root_returns_metadata_json() and the other test functions mentioned (lines 204-214) are missing explicit return type annotations required by the repository's Python coding guidelines. Add `-> None` return type annotation to each test function definition to indicate that these functions do not return any value. This ensures compliance with the guideline that all function parameters and return values must have type hints.Source: Coding guidelines
backend/tests/services/execution/test_request_builder_im_control.py (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
-> Nonereturn annotations to the new test functions.As per coding guidelines,
**/*.py: "Use type hints for all function parameters and return values."Proposed patch
-def test_injects_im_control_skill_for_im_context_task(): +def test_injects_im_control_skill_for_im_context_task() -> None: @@ -def test_does_not_duplicate_im_control_skill(): +def test_does_not_duplicate_im_control_skill() -> None: @@ -def test_task_token_includes_im_context_from_task(): +def test_task_token_includes_im_context_from_task() -> None:Also applies to: 39-39, 50-50
🤖 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/execution/test_request_builder_im_control.py` at line 27, The test functions test_injects_im_control_skill_for_im_context_task, and the other test functions at lines 39 and 50 are missing return type annotations as required by the coding guidelines. Add `-> None` return type annotation to all three test function definitions to comply with the type hints requirement for all function parameters and return values.Source: Coding guidelines
backend/app/mcp_server/server.py (1)
550-569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tracing to the new IM-control MCP helper entry points.
Line 550 and Line 914 introduce new sync helper flows without tracing, which makes MCP startup/config issues harder to diagnose. Please wrap these with sync tracing and add basic span details (server name, registered tool count, config URL target).
As per coding guidelines,
**/*.py: "Use@trace_asyncor@trace_syncdecorators from shared.telemetry.decorators for tracing async and sync functions respectively; use add_span_event() and set_span_attribute() for adding details to spans."Also applies to: 914-929
🤖 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/server.py` around lines 550 - 569, The _register_im_control_tools() function and the similar function at line 914 are missing sync tracing which impairs MCP startup diagnostics. Add the `@trace_sync` decorator from shared.telemetry.decorators to both functions, then use set_span_attribute() to add server name and registered tool count details to the spans, and use add_span_event() to add config URL target information within each function body.Source: Coding guidelines
backend/app/services/execution/request_builder.py (1)
2051-2079: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate IM context normalization to avoid contract drift.
Line 2831 reimplements the same parsing/normalization behavior already added in
backend/app/services/chat/storage/task_manager.py(Line 408 onward). Keeping two copies of this logic risks subtle divergence between persisted task context and token claims. Please move this normalization into one shared helper and reuse it from both paths.Also applies to: 2830-2855
🤖 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/execution/request_builder.py` around lines 2051 - 2079, The _inject_im_control_skill method and the code in backend/app/services/chat/storage/task_manager.py (around line 408) contain duplicate IM context normalization/parsing logic. Create a shared helper function that encapsulates the IM context normalization behavior, then refactor _inject_im_control_skill to use this shared helper instead of _extract_task_im_context, and update task_manager.py to use the same helper as well. This ensures both code paths use identical normalization logic and prevents subtle divergence between persisted task context and token claims.backend/tests/services/chat/storage/test_task_manager.py (1)
349-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type for the new test function.
The new test uses typed parameters but omits a return type; annotate it as
-> Noneto satisfy the typing rule.Proposed fix
def test_create_new_task_writes_im_context_from_message_source( test_db: Session, test_user: User, -): +) -> None:As per coding guidelines, "Use type hints for all function parameters and return values".
🤖 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/storage/test_task_manager.py` around lines 349 - 352, The function test_create_new_task_writes_im_context_from_message_source is missing an explicit return type annotation. Add `-> None` after the closing parenthesis of the function parameters and before the colon to explicitly declare that the test function returns None, which satisfies the typing requirement for all function definitions.Source: Coding guidelines
🤖 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/im_sessions.py`:
- Around line 52-60: The session_bot_purpose value retrieved from
im_session_service.get_session_bot_purpose() may contain unexpected or
unsupported values that will cause serialization to fail. Add normalization
logic for session_bot_purpose after it is retrieved but before it is passed to
the _private_session_out() function to safely handle any unexpected values by
defaulting to a valid fallback value when needed.
In `@backend/app/services/im/control_service.py`:
- Around line 29-223: Add tracing instrumentation to the public async methods in
the IMControlService class that serve as entrypoints. Specifically, decorate the
methods get_current_state, start_new_session, clear_current_session,
confirm_pending_action, and cancel_pending_action with the `@trace_async`
decorator imported from shared.telemetry.decorators. Additionally, add span
attributes using set_span_attribute() within these methods to capture relevant
contextual information such as bot_purpose, session_key, and action type where
applicable, and use add_span_event() to record significant operations like
session saves and cache operations within these entrypoints.
- Around line 127-131: When cancel is called without an action_id, the cached
pending action is not revoked from the cache manager, only the
session.pending_action_id is cleared. This leaves the action confirmable with
the old ID. Modify the logic to ensure that when action_id is not provided, use
the current session.pending_action_id to delete the cache entry before clearing
session.pending_action_id. This ensures the pending action is fully revoked from
both cache and session state.
In `@backend/app/services/im/session_service.py`:
- Around line 144-153: The Kind query being executed in the loop within
list_private_sessions creates an N+1 query problem by executing one database
query per session. Instead of querying the database for each session's
channel_id individually, collect all distinct channel_ids from the sessions
being processed, execute a single bulk query to fetch all matching Kind records
with those channel_ids (filtered by kind == "Messager", user_id == 0, and
is_active == True), and store the results in a local dictionary map keyed by
channel_id. Then reuse this map for each session instead of performing
individual queries, eliminating the per-session database lookups.
- Line 150: The boolean filter comparison in the session_service.py file uses
the equality operator with True, which deviates from SQLAlchemy's recommended
pattern. Locate the filter condition `Kind.is_active == True` and replace it
with `Kind.is_active.is_(True)` to follow SQLAlchemy's boolean comparison idiom,
which provides better clarity and consistency with SQLAlchemy best practices.
In `@backend/init_data/skills/im-control/SKILL.md`:
- Line 49: The documentation in SKILL.md at line 49 incorrectly describes the
tool response condition as `confirmation.required=true`, but the actual IM
control tool response uses `status: "needs_confirmation"` along with
`confirmation.action_id`. Update the description in step 4 to accurately reflect
the actual response shape that triggers the confirmation flow, replacing the
reference to `confirmation.required=true` with the correct `status:
"needs_confirmation"` condition so the documentation matches the implementation.
---
Nitpick comments:
In `@backend/app/api/endpoints/im_sessions.py`:
- Around line 39-46: The async function list_private_sessions is missing the
required `@trace_async` decorator. Add the `@trace_async` decorator imported from
shared.telemetry.decorators above the function definition of
list_private_sessions to enable proper tracing for this async endpoint,
following the coding guidelines that require all async functions to be decorated
with either `@trace_async` or `@trace_sync`.
- Around line 49-55: The code in the loop iterating through sessions calls
get_session_bot_purpose for each session individually, which causes redundant
database queries when multiple sessions belong to the same channel. Refactor the
code to first collect all unique channels from the sessions list, perform a
single batch lookup or mapping operation to get bot purposes for all unique
channels, then use this channel-to-bot-purpose mapping within the loop instead
of calling get_session_bot_purpose repeatedly. This will eliminate duplicate
queries and improve performance as the number of sessions grows.
In `@backend/app/mcp_server/server.py`:
- Around line 550-569: The _register_im_control_tools() function and the similar
function at line 914 are missing sync tracing which impairs MCP startup
diagnostics. Add the `@trace_sync` decorator from shared.telemetry.decorators to
both functions, then use set_span_attribute() to add server name and registered
tool count details to the spans, and use add_span_event() to add config URL
target information within each function body.
In `@backend/app/services/execution/request_builder.py`:
- Around line 2051-2079: The _inject_im_control_skill method and the code in
backend/app/services/chat/storage/task_manager.py (around line 408) contain
duplicate IM context normalization/parsing logic. Create a shared helper
function that encapsulates the IM context normalization behavior, then refactor
_inject_im_control_skill to use this shared helper instead of
_extract_task_im_context, and update task_manager.py to use the same helper as
well. This ensures both code paths use identical normalization logic and
prevents subtle divergence between persisted task context and token claims.
In `@backend/tests/mcp_server/test_server_routes.py`:
- Around line 134-150: The test function
test_im_control_mcp_root_returns_metadata_json() and the other test functions
mentioned (lines 204-214) are missing explicit return type annotations required
by the repository's Python coding guidelines. Add `-> None` return type
annotation to each test function definition to indicate that these functions do
not return any value. This ensures compliance with the guideline that all
function parameters and return values must have type hints.
In `@backend/tests/services/auth/test_task_token_im_context.py`:
- Line 8: Add an explicit `-> None` return type annotation to the
test_task_token_round_trips_im_context function definition to comply with the
coding guideline requiring type hints for all function parameters and return
values. Simply add `-> None` after the function parameters and before the colon.
In `@backend/tests/services/chat/storage/test_task_manager.py`:
- Around line 349-352: The function
test_create_new_task_writes_im_context_from_message_source is missing an
explicit return type annotation. Add `-> None` after the closing parenthesis of
the function parameters and before the colon to explicitly declare that the test
function returns None, which satisfies the typing requirement for all function
definitions.
In `@backend/tests/services/execution/test_request_builder_im_control.py`:
- Line 27: The test functions test_injects_im_control_skill_for_im_context_task,
and the other test functions at lines 39 and 50 are missing return type
annotations as required by the coding guidelines. Add `-> None` return type
annotation to all three test function definitions to comply with the type hints
requirement for all function parameters and return values.
In `@backend/tests/services/im/test_control_service.py`:
- Around line 62-89: Add a new regression test function that verifies the
pending-action revocation contract when canceling without an action_id. Create a
test that: first calls clear_current_session (like the existing
test_confirm_pending_clear_removes_wework_task_binding), then invokes another
operation that cancels the pending action without passing the action_id
parameter, and finally attempts to confirm the pending action using the old
action_id and asserts that this confirmation fails or returns an error status
instead of success. This ensures that once a pending action is revoked through
the cancel-without-action_id pathway, the old action_id can no longer be used
for confirmation.
🪄 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: 95f54a6a-633d-43a2-b3fa-9272a3ddc483
📒 Files selected for processing (32)
backend/app/api/endpoints/admin/im_channels.pybackend/app/api/endpoints/im_sessions.pybackend/app/mcp_server/__init__.pybackend/app/mcp_server/server.pybackend/app/mcp_server/tools/im_control.pybackend/app/models/im_session.pybackend/app/schemas/im_channel.pybackend/app/schemas/im_session.pybackend/app/services/auth/task_token.pybackend/app/services/chat/storage/task_manager.pybackend/app/services/execution/request_builder.pybackend/app/services/im/control_service.pybackend/app/services/im/session_service.pybackend/init_data/skills/im-control/SKILL.mdbackend/tests/api/endpoints/test_admin_im_channels.pybackend/tests/api/endpoints/test_im_sessions_api.pybackend/tests/mcp_server/test_im_control_tools.pybackend/tests/mcp_server/test_server_routes.pybackend/tests/services/auth/test_task_token_im_context.pybackend/tests/services/chat/storage/test_task_manager.pybackend/tests/services/execution/test_request_builder_im_control.pybackend/tests/services/im/test_control_service.pydocs/en/user-guide/integrations/im-channel-integration.mddocs/zh/user-guide/integrations/im-channel-integration.mdwework/src/api/imSessions.test.tswework/src/api/imSessions.tswework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/MobileWorkbenchLayout.test.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/types/api.ts
✅ Files skipped from review due to trivial changes (2)
- docs/zh/user-guide/integrations/im-channel-integration.md
- wework/src/components/layout/MobileWorkbenchLayout.test.tsx
Comment on lines
+52
to
+60
| session_bot_purpose = await im_session_service.get_session_bot_purpose( | ||
| db, | ||
| session, | ||
| ) | ||
| if bot_purpose and session_bot_purpose != bot_purpose: | ||
| continue | ||
| items.append( | ||
| _private_session_out(session, bot_purpose=session_bot_purpose), | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Normalize unsupported bot_purpose values before serialization.
At Line 52, session_bot_purpose is passed through directly. If a channel carries an unexpected persisted value, this path can fail the private-session response instead of safely defaulting.
🛠️ Suggested fix
+SUPPORTED_BOT_PURPOSES = {"wegent_chat", "wework_local"}
+
async def list_private_sessions(
@@
for session in sessions:
- session_bot_purpose = await im_session_service.get_session_bot_purpose(
+ raw_bot_purpose = await im_session_service.get_session_bot_purpose(
db,
session,
)
+ session_bot_purpose = (
+ raw_bot_purpose
+ if raw_bot_purpose in SUPPORTED_BOT_PURPOSES
+ else "wegent_chat"
+ )
if bot_purpose and session_bot_purpose != bot_purpose:
continue🤖 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/im_sessions.py` around lines 52 - 60, The
session_bot_purpose value retrieved from
im_session_service.get_session_bot_purpose() may contain unexpected or
unsupported values that will cause serialization to fail. Add normalization
logic for session_bot_purpose after it is retrieved but before it is passed to
the _private_session_out() function to safely handle any unexpected values by
defaulting to a valid fallback value when needed.
Comment on lines
+29
to
+223
| class IMControlService: | ||
| """Single service for mutating private IM control state.""" | ||
|
|
||
| async def get_current_state( | ||
| self, | ||
| db: Session | None, | ||
| *, | ||
| session: IMPrivateSession, | ||
| bot_purpose: str, | ||
| ) -> dict[str, Any]: | ||
| return self._success( | ||
| "当前状态已获取。", | ||
| state=self._build_state(session, bot_purpose), | ||
| ) | ||
|
|
||
| async def start_new_session( | ||
| self, | ||
| db: Session | None, | ||
| *, | ||
| session: IMPrivateSession, | ||
| bot_purpose: str, | ||
| ) -> dict[str, Any]: | ||
| session.mode = ( | ||
| IMSessionMode.CHAT if bot_purpose == "wegent_chat" else IMSessionMode.TASK | ||
| ) | ||
| session.active_task_id = None | ||
| session.active_runtime_task = None | ||
| session.current_target_type = ( | ||
| "conversation" | ||
| if bot_purpose == "wegent_chat" | ||
| else "wework_local_conversation" | ||
| ) | ||
| session.current_target = {} | ||
| session.pending_action_id = None | ||
| await im_session_service.save_session(session) | ||
| message = ( | ||
| "已开始新对话。" | ||
| if bot_purpose == "wegent_chat" | ||
| else "已开始新的 Wework 本地对话。" | ||
| ) | ||
| return self._success(message, state=self._build_state(session, bot_purpose)) | ||
|
|
||
| async def clear_current_session( | ||
| self, | ||
| db: Session | None, | ||
| *, | ||
| session: IMPrivateSession, | ||
| bot_purpose: str, | ||
| ) -> dict[str, Any]: | ||
| if bot_purpose == "wework_local" and session.active_runtime_task: | ||
| action_id = await self._store_pending_action( | ||
| session, | ||
| {"type": "clear_current_session", "bot_purpose": bot_purpose}, | ||
| ) | ||
| session.pending_action_id = action_id | ||
| await im_session_service.save_session(session) | ||
| return { | ||
| "status": "needs_confirmation", | ||
| "message": "这会清除当前 Wework 任务绑定。", | ||
| "state": self._build_state(session, bot_purpose), | ||
| "confirmation": { | ||
| "action_id": action_id, | ||
| "summary": "清除当前 Wework 任务绑定", | ||
| }, | ||
| } | ||
| return await self._clear_now(db, session=session, bot_purpose=bot_purpose) | ||
|
|
||
| async def confirm_pending_action( | ||
| self, | ||
| db: Session | None, | ||
| *, | ||
| session: IMPrivateSession, | ||
| bot_purpose: str, | ||
| action_id: str, | ||
| ) -> dict[str, Any]: | ||
| action = await cache_manager.get(self._pending_action_key(action_id)) | ||
| if ( | ||
| not isinstance(action, dict) | ||
| or action.get("session_key") != session.session_key | ||
| ): | ||
| return self._error("确认操作已过期，请重新发起。", session, bot_purpose) | ||
| if action.get("type") == "clear_current_session": | ||
| await cache_manager.delete(self._pending_action_key(action_id)) | ||
| return await self._clear_now( | ||
| db, | ||
| session=session, | ||
| bot_purpose=bot_purpose, | ||
| ) | ||
| return self._error("无法识别的确认操作。", session, bot_purpose) | ||
|
|
||
| async def cancel_pending_action( | ||
| self, | ||
| db: Session | None, | ||
| *, | ||
| session: IMPrivateSession, | ||
| bot_purpose: str, | ||
| action_id: str | None = None, | ||
| ) -> dict[str, Any]: | ||
| if action_id: | ||
| await cache_manager.delete(self._pending_action_key(action_id)) | ||
| session.pending_action_id = None | ||
| await im_session_service.save_session(session) | ||
| return self._success("已取消。", state=self._build_state(session, bot_purpose)) | ||
|
|
||
| async def _clear_now( | ||
| self, | ||
| db: Session | None, | ||
| *, | ||
| session: IMPrivateSession, | ||
| bot_purpose: str, | ||
| ) -> dict[str, Any]: | ||
| await im_session_service.clear_active_task(db, session=session) | ||
| session.current_target_type = None | ||
| session.current_target = None | ||
| session.pending_action_id = None | ||
| await im_session_service.save_session(session) | ||
| return self._success( | ||
| "已清除当前会话。", state=self._build_state(session, bot_purpose) | ||
| ) | ||
|
|
||
| async def _store_pending_action( | ||
| self, | ||
| session: IMPrivateSession, | ||
| payload: dict[str, Any], | ||
| ) -> str: | ||
| action_id = f"im_action_{uuid4().hex}" | ||
| await cache_manager.set( | ||
| self._pending_action_key(action_id), | ||
| {"session_key": session.session_key, **payload}, | ||
| expire=PENDING_ACTION_TTL_SECONDS, | ||
| ) | ||
| return action_id | ||
|
|
||
| def _pending_action_key(self, action_id: str) -> str: | ||
| return f"{PENDING_ACTION_PREFIX}{action_id}" | ||
|
|
||
| def _build_state( | ||
| self, | ||
| session: IMPrivateSession, | ||
| bot_purpose: str, | ||
| ) -> dict[str, Any]: | ||
| target_label = "" | ||
| if isinstance(session.active_runtime_task, dict): | ||
| target_label = str( | ||
| session.active_runtime_task.get("title") | ||
| or session.active_runtime_task.get("localTaskId") | ||
| or "" | ||
| ) | ||
| elif session.active_task_id: | ||
| target_label = f"Task {session.active_task_id}" | ||
| return { | ||
| "bot_purpose": bot_purpose, | ||
| "mode": session.mode, | ||
| "state": session.state, | ||
| "current_target_type": self._current_target_type(session), | ||
| "current_target_label": target_label, | ||
| "pending_action_id": session.pending_action_id, | ||
| "available_actions": [ | ||
| "get_current_state", | ||
| "start_new_session", | ||
| "clear_current_session", | ||
| "cancel_pending_action", | ||
| ], | ||
| } | ||
|
|
||
| def _current_target_type(self, session: IMPrivateSession) -> str | None: | ||
| if session.active_runtime_task: | ||
| return "wework_runtime_task" | ||
| if session.active_task_id: | ||
| return "wegent_task" | ||
| return session.current_target_type | ||
|
|
||
| def _success(self, message: str, *, state: dict[str, Any]) -> dict[str, Any]: | ||
| return { | ||
| "status": "success", | ||
| "message": message, | ||
| "state": state, | ||
| "confirmation": None, | ||
| } | ||
|
|
||
| def _error( | ||
| self, | ||
| message: str, | ||
| session: IMPrivateSession, | ||
| bot_purpose: str, | ||
| ) -> dict[str, Any]: | ||
| return { | ||
| "status": "error", | ||
| "message": message, | ||
| "state": self._build_state(session, bot_purpose), | ||
| "confirmation": None, | ||
| } | ||
|
|
||
|
|
||
| im_control_service = IMControlService() |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add tracing decorators to the new control-service entrypoints.
The new async control operations are currently untraced. Please instrument them with the shared telemetry decorators and span metadata hooks required by repo standards.
As per coding guidelines: "Use @trace_async or @trace_sync decorators from shared.telemetry.decorators for tracing async and sync functions respectively; use add_span_event() and set_span_attribute() for adding details to spans."
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 109-109: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
🤖 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/im/control_service.py` around lines 29 - 223, Add
tracing instrumentation to the public async methods in the IMControlService
class that serve as entrypoints. Specifically, decorate the methods
get_current_state, start_new_session, clear_current_session,
confirm_pending_action, and cancel_pending_action with the `@trace_async`
decorator imported from shared.telemetry.decorators. Additionally, add span
attributes using set_span_attribute() within these methods to capture relevant
contextual information such as bot_purpose, session_key, and action type where
applicable, and use add_span_event() to record significant operations like
session saves and cache operations within these entrypoints.
Source: Coding guidelines
Comment on lines
+127
to
+131
| if action_id: | ||
| await cache_manager.delete(self._pending_action_key(action_id)) | ||
| session.pending_action_id = None | ||
| await im_session_service.save_session(session) | ||
| return self._success("已取消。", state=self._build_state(session, bot_purpose)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cancel without action_id does not revoke the active pending action.
When action_id is omitted, only session.pending_action_id is cleared; the cached pending action remains confirmable via confirm_pending_action using the old ID. Cancel should revoke action_id or the current session.pending_action_id.
Suggested fix
async def cancel_pending_action(
self,
db: Session | None,
*,
session: IMPrivateSession,
bot_purpose: str,
action_id: str | None = None,
) -> dict[str, Any]:
- if action_id:
- await cache_manager.delete(self._pending_action_key(action_id))
+ action_to_cancel = action_id or session.pending_action_id
+ if action_to_cancel:
+ await cache_manager.delete(self._pending_action_key(action_to_cancel))
session.pending_action_id = None
await im_session_service.save_session(session)
return self._success("已取消。", state=self._build_state(session, bot_purpose))🤖 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/im/control_service.py` around lines 127 - 131, When
cancel is called without an action_id, the cached pending action is not revoked
from the cache manager, only the session.pending_action_id is cleared. This
leaves the action confirmable with the old ID. Modify the logic to ensure that
when action_id is not provided, use the current session.pending_action_id to
delete the cache entry before clearing session.pending_action_id. This ensures
the pending action is fully revoked from both cache and session state.
Comment on lines
+144
to
+153
| channel = ( | ||
| db.query(Kind) | ||
| .filter( | ||
| Kind.id == session.channel_id, | ||
| Kind.kind == "Messager", | ||
| Kind.user_id == 0, | ||
| Kind.is_active == True, | ||
| ) | ||
| .first() | ||
| ) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid per-session bot-purpose DB lookups in listing paths.
This method performs one SQL query per session, and list_private_sessions calls it inside a session loop, creating an N+1 query pattern that scales linearly with user session count. Consider resolving bot purposes in bulk for distinct channel_ids and reusing a local map.
🧰 Tools
🪛 Ruff (0.15.18)
[error] 150-150: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
🤖 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/im/session_service.py` around lines 144 - 153, The Kind
query being executed in the loop within list_private_sessions creates an N+1
query problem by executing one database query per session. Instead of querying
the database for each session's channel_id individually, collect all distinct
channel_ids from the sessions being processed, execute a single bulk query to
fetch all matching Kind records with those channel_ids (filtered by kind ==
"Messager", user_id == 0, and is_active == True), and store the results in a
local dictionary map keyed by channel_id. Then reuse this map for each session
instead of performing individual queries, eliminating the per-session database
lookups.
| Kind.id == session.channel_id, | ||
| Kind.kind == "Messager", | ||
| Kind.user_id == 0, | ||
| Kind.is_active == True, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and inspect the relevant lines
cat -n backend/app/services/im/session_service.py | sed -n '145,155p'Repository: wecode-ai/Wegent
Length of output: 586
🏁 Script executed:
# Check if there's a Ruff configuration in the project
fd -e "pyproject.toml" -e ".ruff.toml" -e "ruff.toml" | head -5Repository: wecode-ai/Wegent
Length of output: 154
🏁 Script executed:
# Look for the Kind import and understand the context better
rg "class Kind" backend/app/services/im/session_service.py -B 2 -A 5Repository: wecode-ai/Wegent
Length of output: 154
🏁 Script executed:
# Check for imports and see if SQLAlchemy is being used
head -50 backend/app/services/im/session_service.pyRepository: wecode-ai/Wegent
Length of output: 1655
🏁 Script executed:
# Find the Kind model definition
rg "class Kind" backend/ -lRepository: wecode-ai/Wegent
Length of output: 292
🏁 Script executed:
# Check Ruff configuration in the project
cat pyproject.toml | grep -A 20 "\[tool.ruff"Repository: wecode-ai/Wegent
Length of output: 154
🏁 Script executed:
# Look at a broader context of the session_service.py to understand imports
head -20 backend/app/services/im/session_service.pyRepository: wecode-ai/Wegent
Length of output: 667
🏁 Script executed:
# Search for Kind import in the file
rg "^from|^import" backend/app/services/im/session_service.py | head -20Repository: wecode-ai/Wegent
Length of output: 450
🏁 Script executed:
# Search for where Kind is imported in session_service.py
rg "Kind" backend/app/services/im/session_service.py | head -20Repository: wecode-ai/Wegent
Length of output: 385
🏁 Script executed:
# Let's look at more of the session_service.py to find the Kind import
sed -n '1,200p' backend/app/services/im/session_service.py | grep -E "^from|^import|Kind"Repository: wecode-ai/Wegent
Length of output: 681
🏁 Script executed:
# Check the Kind model definition - it's likely in app.models
rg "class Kind" backend/app/models/ -A 10Repository: wecode-ai/Wegent
Length of output: 154
🏁 Script executed:
# Find the Kind model - it was imported from app.models.kind
cat -n backend/app/models/kind.py | head -100Repository: wecode-ai/Wegent
Length of output: 516
🏁 Script executed:
# Check if Ruff is configured and if E712 is enabled
find backend -name "pyproject.toml" -o -name ".ruff.toml" -o -name "ruff.toml" | xargs cat 2>/dev/nullRepository: wecode-ai/Wegent
Length of output: 6020
🏁 Script executed:
# Look for the full function containing line 150 to understand the context better
sed -n '135,165p' backend/app/services/im/session_service.pyRepository: wecode-ai/Wegent
Length of output: 1177
🏁 Script executed:
# Search for any Ruff configuration files in the entire project
find backend -name "*ruff*" -o -name "pyproject.toml" | xargs grep -l "ruff" 2>/dev/null || echo "No ruff configuration found"Repository: wecode-ai/Wegent
Length of output: 182
🏁 Script executed:
# Check if ruff is mentioned anywhere in the project
rg "ruff" backend/ root/ --type-list py 2>/dev/null | head -20Repository: wecode-ai/Wegent
Length of output: 154
🏁 Script executed:
# Let's verify what the SQLAlchemy API suggests for boolean comparisons
# Check if there's any SQLAlchemy usage pattern in the codebase with boolean columns
rg "\.is_\(|== True|== False" backend/ --type py | head -20Repository: wecode-ai/Wegent
Length of output: 2231
Use SQLAlchemy boolean-filter idiom for clarity and maintainability.
Replace Kind.is_active == True with Kind.is_active.is_(True). This follows SQLAlchemy's recommended pattern for boolean column comparisons and improves code clarity.
🧰 Tools
🪛 Ruff (0.15.18)
[error] 150-150: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
🤖 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/im/session_service.py` at line 150, The boolean filter
comparison in the session_service.py file uses the equality operator with True,
which deviates from SQLAlchemy's recommended pattern. Locate the filter
condition `Kind.is_active == True` and replace it with
`Kind.is_active.is_(True)` to follow SQLAlchemy's boolean comparison idiom,
which provides better clarity and consistency with SQLAlchemy best practices.
Source: Linters/SAST tools
| 1. Prefer these tools over explaining slash commands. | ||
| 2. If the user asks what the IM bot is currently connected to, call `im_control_get_current_state` before answering. | ||
| 3. If the user asks to restart, stop continuing the previous task, or detach from the current task, call `im_control_clear_current_session`. | ||
| 4. If a tool returns `confirmation.required=true`, ask the user to confirm in plain language and include the returned `action_id` only in your internal follow-up tool call. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the confirmation condition to match actual tool response shape.
Line 49 checks confirmation.required=true, but the IM control response shape uses status: "needs_confirmation" plus confirmation.action_id. This mismatch can skip the confirmation flow in practice.
💡 Suggested doc fix
-4. If a tool returns `confirmation.required=true`, ask the user to confirm in plain language and include the returned `action_id` only in your internal follow-up tool call.
+4. If a tool returns `status="needs_confirmation"`, ask the user to confirm in plain language and include `confirmation.action_id` only in your internal follow-up tool call.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 4. If a tool returns `confirmation.required=true`, ask the user to confirm in plain language and include the returned `action_id` only in your internal follow-up tool call. | |
| 4. If a tool returns `status="needs_confirmation"`, ask the user to confirm in plain language and include `confirmation.action_id` only in your internal follow-up tool call. |
🧰 Tools
🪛 LanguageTool
[style] ~49-~49: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... im_control_clear_current_session. 4. If a tool returns `confirmation.required=t...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 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/init_data/skills/im-control/SKILL.md` at line 49, The documentation
in SKILL.md at line 49 incorrectly describes the tool response condition as
`confirmation.required=true`, but the actual IM control tool response uses
`status: "needs_confirmation"` along with `confirmation.action_id`. Update the
description in step 4 to accurately reflect the actual response shape that
triggers the confirmation flow, replacing the reference to
`confirmation.required=true` with the correct `status: "needs_confirmation"`
condition so the documentation matches the implementation.
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
1 participant
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
Validation
Summary by CodeRabbit