[Reflect] RF-3755: Release new MCP tools for creating and editing tests. Add new SAP-specific test creation skill.#369
Conversation
| const response = (await responsePromise) as Record<string, unknown>; | ||
| const result = response.result as Record<string, unknown> | undefined; |
There was a problem hiding this comment.
You should just cast as the correct type
| const response = (await responsePromise) as Record<string, unknown>; | |
| const result = response.result as Record<string, unknown> | undefined; | |
| const response = (await responsePromise) as { result?: { typ?: string; response?: string | boolean } }; | |
| const result = response.result; |
src/reflect/prompts.ts
Outdated
| import type { PromptParams } from "../common/types"; | ||
|
|
||
| export const PROMPTS: PromptParams[] = [ | ||
| { |
There was a problem hiding this comment.
Should follow the same 1 file per entry as the tools IMO rather than starting a non scaling pattern again and then refactoring later
| private activeConnections = new Map<string, WebSocketManager>(); | ||
| private sessionStates = new Map< | ||
| string, | ||
| { platform: TestPlatform; test: { name: string } } | ||
| >(); |
There was a problem hiding this comment.
Do we need to store activeConnections and sessionStates on the shared ReflectClient instance here?
I’m asking because ReflectClient keeps long-lived mutable state (apiToken, activeConnections, sessionStates) on the client instance, while the clients are registered as singletons and the same instance is reused across HTTP MCP sessions. This means one HTTP session could overwrite another session’s Reflect auth and potentially inherit or leak live recording session sockets.
Also, when a session closes, it only removes the transport, it doesn’t clear those Reflect-held connections. Let me know if I misunderstood completely.
There was a problem hiding this comment.
Updated the server.ts to expose a "cleanup" function and am using it to clean up the websocket connections now. I think this only matters when running the MCP server is HTTP mode since in stdio you can only ever have one session.
| waitForResponse(id: string): Promise<unknown> { | ||
| return new Promise((resolve, reject) => { | ||
| this.pendingResponses.set(id, { resolve, reject }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
What should happen here if the websocket disconnects or Reflect never sends a response for this request? Should we add timeout/cleanup logic so the tool call doesn’t hang forever pendingResponses? I think adding a close handler that rejects all pending promises and cleans up state would prevent from hanging.
There was a problem hiding this comment.
I originally had timeout logic here but removed it since some operations were taking longer than the timeout but succeeding in Reflect, but were ending up being considered failed in the MCP server because the timeout was exceeded. We could potentially add reconnect to make it resilient to websocket disconnects, but there might be value in keeping the code simple for now and adding it once we see evidence that websocket reconnect logic is something that would be beneficial to users in practice. For now, given that websockets would only be valid for the length of a recording session, a user can recover by ending the recording session and restarting it...
| const response = (await responsePromise) as Record<string, unknown>; | ||
| const result = response.result as Record<string, unknown> | undefined; |
There was a problem hiding this comment.
[nit] Add casting the response to MCPAddPromptStepSuccessResponse (which you already have defined!)
|
@sazap10 This is ready for final review and approval |
|
@domarmstrong Are you able to approve and merge this? |
tmcneal
force-pushed
the
RF-3755-2
branch
2 times, most recently
from
cea58d9 to
318b30c
Compare
This adds a new set of MCP tools that allows external agents to interact with a live Web or Mobile recording session. This enables an external agent to assist in creating and editing tests. Also includes an update to the base server.ts to support cleaning up MCP session-related items when a session ends. This is required so that we can cleanup and close WebSockets associated with a given session when we're running in HTTP mode.
…tests This populates the MCP server "instructions" property with guidance for how the agent should approach creating and editing tests in Reflect.
Head branch was pushed to by a user without write access
Via the "prompts" property, we expose a skill that gives the agent additional guidance when creating Reflect tests for SAP BTP apps and SAP S4/HANA.
4 participants
Goal
This adds additional MCP tools and associated instructions to enable Agents to assist Testers in creating and modifying Reflect tests. Additionally, it exposes a new "skill" in the form of a Prompt definition that adds additional instructions for when a user wants to test an SAP application.
Design
The
list_segmentstool uses the Reflect API just like the existing tools. The other tools interact with Reflect via a persistent WebSocket connection. This connection is made directly to the test session that's running in our Cloud infrastructure. This works the same way for Web tests and Mobile tests. After the WebSocket connection is established viaconnect_to_session, tools can be invoked which issue commands on the WebSocket connection.The business logic for each of these WebSocket commands lies within Reflect's infrastructure. This means that the logic in the MCP server is pretty simple - its main responsibilities are maintaining the socket connection, emitting messages on the socket, waiting for messages on the socket, and error handling.
Changeset
The following tools are added:
Additionally, the
instructionsproperty is now updated with guidance to the Agent on how to properly build a Reflect test.Finally, a new skill is added that's specific to testing SAP apps.
Testing
reflect-sap-testskill