feat(session): add attach/detach support with owned/attached session …#287
feat(session): add attach/detach support with owned/attached session …#287tgz0514 wants to merge 2 commits intoappium:mainfrom
Conversation
…ownership - Integrate attach and detach into unified session management - Track session ownership in the session store; created sessions are owned - Reject deleting attached sessions; disconnect cleans up only owned sessions - Expose ownership in session listing output Fixes appium#274
| @@ -1,212 +0,0 @@ | |||
| # Tools Directory | |||
There was a problem hiding this comment.
Can you separate this change from this PR?
| import { errorResult, textResult, toolErrorMessage } from '../tool-response.js'; | ||
|
|
||
| export async function deleteSessionAction(sessionId?: string): Promise<any> { | ||
| const ownership = getSessionOwnership(sessionId); |
There was a problem hiding this comment.
So, based on the current implementation, users cannot delete attached sessions, correct?
I wondered if "delete" itself allows users to call the delete session since in this case, users may request MCP to 'delete' the session. While MCP will not delete automatically in safeDeleteAllSessions only - which is triggered when MCP server ends.
There was a problem hiding this comment.
Correct. In the current implementation, attached sessions cannot be deleted with delete.
This is an intentional ownership model: sessions created through create are owned by MCP Appium, so they can be deleted; sessions brought in through attach are effectively borrowed external sessions, and MCP Appium should not take over their lifecycle. So if a user tries to remove an attached session, the current behavior is to return an error and direct them to use action=detach instead. detach only disconnects MCP Appium's management of that session and does not terminate the actual remote Appium session.
In other words, both manual delete and server-shutdown cleanup through safeDeleteAllSessions only delete owned sessions.
There was a problem hiding this comment.
Then, could you keep the logic only for safeDeleteAllSessions? So, as regular WebDriver client behavior, Appium MCP will delete the session if the client explicitly request Appium MCP to detele the session
There was a problem hiding this comment.
I’m leaning toward adding an argument to safeDeleteAllSessions(), with the default behavior cleaning up only owned sessions and skipping attached sessions. This would make the current filtering explicit while keeping the change in the internal helper layer, so it would not affect the appium_session_management parameter list.
By contrast, if I add another parameter to the delete action, that parameter would be exposed in the appium_session_management schema. That would make the tool surface larger and add more complexity for AI clients.
I also still have some safety concerns about allowing delete to terminate attached sessions directly. Attached sessions are not created or owned by MCP Appium; they are externally owned sessions that MCP Appium only connects to. If an AI client passes the wrong sessionId or action, deleting an attached session could affect the external session owner. Because of that, I currently prefer to keep the ownership-based restriction: owned sessions can be deleted, while attached sessions should use detach.
If you think the regular delete action should still follow the default WebDriver deletion semantics, I can adjust in that direction as well.
There was a problem hiding this comment.
It sounds like LLM itself should not call deleteSessionAction for attached owner case, then instead of preventing users from deleting attached sessions, even though they may want to...?
The tools' description should let LLM check the session's ownership first for delete/detach's possible prompt. If the ownership is attached, the LLM will call the detach action. Only if a user explicitly prompts to call "delete the session instead of detach" (like this) will the LLM call delete without considering ownership
There was a problem hiding this comment.
I’d like to confirm your expected delete semantics before I update this part.
As I see it, there are two possible directions:
Keep delete without any extra parameter, and let it close all session types by default, including both owned and attached.
In this model, ownership is mainly used to help the LLM choose between detach and delete by default, but it does not restrict an explicit delete call.
Add an extra parameter to delete, so the caller explicitly passes the session type or deletion scope, and the implementation decides based on that parameter whether deletion should be allowed.
This would make the behavior more explicit, but it would also expose another parameter in the appium_session_management schema and make the tool surface more complex.
Which direction do you prefer?
I want to confirm this first before I make further changes to the delete path.
There was a problem hiding this comment.
I'd like to see the former one, so let's LLM detect the direction
| @@ -269,7 +304,9 @@ export async function safeDeleteSession(sessionId?: string): Promise<boolean> { | |||
|
|
|||
| export async function safeDeleteAllSessions(): Promise<number> { | |||
There was a problem hiding this comment.
Could you define a new name of method, or add an argument to filter out non-owned sessions to delete instead of silently ignoring attached sessions?
| : undefined; | ||
|
|
||
| const client = await ( | ||
| WebDriver as unknown as { |
There was a problem hiding this comment.
WebDriver type didn't resolve the method on your env?
| ...(user && key ? { user, key } : {}), | ||
| }); | ||
|
|
||
| const capabilities = args.capabilities ?? getAttachedCapabilities(client); |
There was a problem hiding this comment.
Could you try:
(driver as Client).getAppiumSessionCapabilities()(driver as Client).getSession()args.capabilities
order? Then, what about giving the capabilities to attachToSession's option.
webdriver has some internal logic to change the behavior depending on capabilities, so this would help to let webdriver instance behave better
|
|
||
| const capabilities = args.capabilities ?? getAttachedCapabilities(client); | ||
| setSession( | ||
| client as Parameters<typeof setSession>[0], |
There was a problem hiding this comment.
The client can be WebDriver type
| return nextSession ?? null; | ||
| } | ||
|
|
||
| export function detachSession(sessionId?: string): boolean { |
There was a problem hiding this comment.
i think we should check the ownership of the detach session else any internal request may detach session accidentally
There was a problem hiding this comment.
Also what happens when you attach a sessionId thats already in the store?
…ownership
Fixes #279