Skip to content

Jules was unable to complete the task in time. Please review the work… #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
james2037 wants to merge 1 commit into from
Closed

Jules was unable to complete the task in time. Please review the work… #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 69 additions & 57 deletions src/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,72 +203,58 @@
$responsePayload = null;

try {
// HttpTransport->receive() returns array (decoded JSON) or throws TransportException.
// It ensures the payload is a valid JSON object or array of objects.
$rawPayload = $httpTransport->receive();

$responseMessages = [];
// HttpTransport's receive method ensures $rawPayload is either an associative array (single)
// or a non-empty list of associative arrays (batch).
$isBatchRequest = array_is_list($rawPayload);

if ($isBatchRequest) {
foreach ($rawPayload as $rawMessage) {
// $rawMessage is expected to be an associative array here.
try {
$messageJson = json_encode($rawMessage);
if ($messageJson === false) {
// This should be rare if $rawMessage is a valid array from json_decode
throw new \RuntimeException("Failed to re-encode batch item for parsing.", JsonRpcMessage::PARSE_ERROR);
}
$message = JsonRpcMessage::fromJson($messageJson);
$response = $this->processSingleMessage($message);
if ($response) {
$responseMessages[] = $response;
}
} catch (\Throwable $e) {
$id = is_array($rawMessage) && array_key_exists('id', $rawMessage) ? $rawMessage['id'] : null;
// Ensure ID is string or null for JsonRpcMessage::error
$id = is_scalar($id) ? (string)$id : null;
$responseMessages[] = JsonRpcMessage::error(JsonRpcMessage::INTERNAL_ERROR, "Error processing item in batch: " . $e->getMessage(), $id);
}
}
$responsePayload = !empty($responseMessages) ? $responseMessages : null;
} else { // Single request (which is an associative array)
try {
$messageJson = json_encode($rawPayload);
if ($messageJson === false) {
throw new \RuntimeException("Failed to re-encode single request for parsing.", JsonRpcMessage::PARSE_ERROR);
}
$message = JsonRpcMessage::fromJson($messageJson);
$responsePayload = $this->processSingleMessage($message);
} catch (\Throwable $e) {
$id = is_array($rawPayload) && array_key_exists('id', $rawPayload) ? $rawPayload['id'] : null;
// Ensure ID is string or null for JsonRpcMessage::error
$id = is_scalar($id) ? (string)$id : null;
$errorCode = JsonRpcMessage::INTERNAL_ERROR; // Default
if ($e instanceof \MCP\Server\Exception\MethodNotSupportedException) {
$errorCode = JsonRpcMessage::METHOD_NOT_FOUND;
} elseif ($e instanceof \MCP\Server\Exception\InvalidRequestException) {
$errorCode = JsonRpcMessage::INVALID_REQUEST;
} elseif ($e instanceof \MCP\Server\Exception\InvalidParamsException) {
$errorCode = JsonRpcMessage::INVALID_PARAMS;
} elseif ($e instanceof \RuntimeException && $e->getCode() !== 0 && is_int($e->getCode())) {
$errorCode = $e->getCode();
}
$responsePayload = JsonRpcMessage::error($errorCode, "Error processing request: " . $e->getMessage(), $id);
// HttpTransport::receive() now returns JsonRpcMessage[]|null|false or throws TransportException.
$messagesReceived = $httpTransport->receive();
$isBatchResponse = false; // Default: assume response is not a batch unless determined otherwise.

if ($messagesReceived === false) {
// Transport validation failed (e.g., wrong HTTP method, wrong Content-Type).
$this->logMessage('error', 'HttpTransport->receive() returned false, request unsuitable.', 'Server.runHttpRequestCycle');
$responsePayload = JsonRpcMessage::error(JsonRpcMessage::INVALID_REQUEST, 'Invalid request (transport level error).', null);

Check warning on line 213 in src/Server.php

View check run for this annotation

Codecov / codecov/patch

src/Server.php#L212-L213 

Added lines #L212 - L213 were not covered by tests
} elseif ($messagesReceived === null) {
// Empty or whitespace-only body, parseMessages returned null.
$this->logMessage('error', 'HttpTransport->receive() returned null (empty request body).', 'Server.runHttpRequestCycle');
$responsePayload = JsonRpcMessage::error(JsonRpcMessage::INVALID_REQUEST, 'Request body was empty or contained only whitespace.', null);

Check warning on line 217 in src/Server.php

View check run for this annotation

Codecov / codecov/patch

src/Server.php#L216-L217 

Added lines #L216 - L217 were not covered by tests
} else {
// $messagesReceived is JsonRpcMessage[] (can be an empty array for "[]" batch from parseMessages)
$isBatchResponse = $httpTransport->lastRequestAppearedAsBatch();
$responseMessages = $this->processMessageBatch($messagesReceived); // Use the new helper

if ($isBatchResponse) {
// For a batch request, always respond with an array of responses.
// $responseMessages is already JsonRpcMessage[] from processMessageBatch.
// If all were notifications, $responseMessages will be empty, resulting in "[]" response.
$responsePayload = $responseMessages;
} elseif (!empty($responseMessages)) {
// Single request that yielded a response.
$responsePayload = $responseMessages[0];
} else {
// Single request that was a notification (no response generated),
// or an empty batch "[]" that didn't appear as a batch (edge case, defensive).
// $responsePayload remains null.
$responsePayload = null;
}
}
} catch (TransportException $e) {
$this->logMessage('error', 'TransportException in HTTP cycle: ' . $e->getMessage(), 'Server.runHttpRequestCycle', ['code' => $e->getCode()]);
$errorCode = (is_int($e->getCode()) && $e->getCode() !== 0) ? $e->getCode() : JsonRpcMessage::INTERNAL_ERROR;
$responsePayload = JsonRpcMessage::error($errorCode, $e->getMessage(), null); // ID is null for transport errors not tied to a specific request ID
$responsePayload = JsonRpcMessage::error($errorCode, $e->getMessage(), null);
$isBatchResponse = false; // Not a batch response in case of these transport errors before processing
} catch (\Throwable $e) { // Catchall for unexpected errors
$this->logMessage('critical', "Critical Server Error in HTTP cycle: " . $e->getMessage(), 'Server.runHttpRequestCycle', ['trace' => $e->getTraceAsString()]);
$responsePayload = JsonRpcMessage::error(JsonRpcMessage::INTERNAL_ERROR, 'Internal server error.', null); // ID is null
$responsePayload = JsonRpcMessage::error(JsonRpcMessage::INTERNAL_ERROR, 'Internal server error.', null);
$isBatchResponse = false; // Not a batch response for critical internal errors

Check warning on line 246 in src/Server.php

View check run for this annotation

Codecov / codecov/patch

src/Server.php#L245-L246 

Added lines #L245 - L246 were not covered by tests
}

$httpTransport->send($responsePayload); // $responsePayload could be single JsonRpcMessage or array of them, or null for no response
// Only call send if there's a non-null payload OR if it's a batch response (which might be an empty array for all-notification batches).
if ($responsePayload !== null || $isBatchResponse) {
/** @var JsonRpcMessage|JsonRpcMessage[] $responsePayload PHPStan type hint */
// If $isBatchResponse is true, $responsePayload is guaranteed to be an array (possibly empty from processMessageBatch).
// If $responsePayload is not null and $isBatchResponse is false, it's a JsonRpcMessage.
$httpTransport->send($responsePayload);
}
// If $responsePayload is null AND $isBatchResponse is false (e.g. single notification), send() is not called.
// HttpTransport->getResponse() will then provide a default (e.g. HTTP 204 or empty 200).
$httpResponse = $httpTransport->getResponse(); // This will always return a ResponseInterface

if (!headers_sent()) {
Expand All @@ -293,6 +279,32 @@
}
}

/**
* Processes a batch of received JsonRpcMessage objects.
*
* @param JsonRpcMessage[] $receivedMessages An array of messages to process.
* This array can be empty if an empty batch "[]" was received.
* @return JsonRpcMessage[] An array of response messages. Empty if all messages were notifications or errors for notifications.
*/
private function processMessageBatch(array $receivedMessages): array
{
$responseMessages = [];
if (empty($receivedMessages)) {
// If an empty batch "[]" was received and parsed by AbstractTransport::parseMessages,
// $receivedMessages will be an empty array. No processing needed.
return [];

Check warning on line 295 in src/Server.php

View check run for this annotation

Codecov / codecov/patch

src/Server.php#L295 

Added line #L295 was not covered by tests
}

foreach ($receivedMessages as $currentMessage) {
// Type safety: $currentMessage should already be JsonRpcMessage
// due to the return type of AbstractTransport::parseMessages.
$response = $this->processSingleMessage($currentMessage);
if ($response) {
$responseMessages[] = $response;
}
}
return $responseMessages;
}

/**
* Shuts down the server and its capabilities.
Expand Down
125 changes: 125 additions & 0 deletions src/Transport/AbstractTransport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,134 @@
return $json;
}

/**
* Parses a raw string which may contain one or more JSON-RPC messages.
*
* This method attempts to decode the incoming raw data string. It expects
* the raw data to be either a single JSON object (a single JSON-RPC request)
* or a JSON array of JSON objects (a batch of JSON-RPC requests).
*
* - If $rawData represents a single JSON-RPC message, it's wrapped in an array.
* - If $rawData represents a batch of JSON-RPC messages, each is processed.
* - If $rawData is empty after trim, returns `null`.
* - If $rawData has malformed JSON, throws `TransportException` with code `JsonRpcMessage::PARSE_ERROR`.
* - If $rawData is valid JSON but not a valid JSON-RPC structure (e.g. not an object/array,
* or messages within a batch are invalid), throws `TransportException` with code `JsonRpcMessage::INVALID_REQUEST`.
*
* @param string $rawData The raw string data received from the transport.
* @return JsonRpcMessage[]|null An array of JsonRpcMessage objects if parsing is successful and input is not empty,
* or null if $rawData is empty after trimming.
* @throws TransportException If parsing fails due to malformed JSON, invalid JSON-RPC structure, or excessive size.
*/
protected function parseMessages(string $rawData): ?array
{
$trimmedData = trim($rawData);
if (empty($trimmedData)) {
// As per requirement: "If $rawData is empty after trim, return null."
return null;
}

if (strlen($trimmedData) > static::MAX_MESSAGE_SIZE) {
throw new TransportException(
"Raw data exceeds size limit of " . static::MAX_MESSAGE_SIZE . " bytes.",
JsonRpcMessage::INVALID_REQUEST
);
}

$decodedData = json_decode($trimmedData, true);
$jsonErrorCode = json_last_error();

if ($jsonErrorCode !== JSON_ERROR_NONE) {
throw new TransportException(
"Failed to decode JSON: " . json_last_error_msg() . ". Data: " . substr($trimmedData, 0, 200) . "...",
JsonRpcMessage::PARSE_ERROR
);
}

if (!is_array($decodedData)) {
throw new TransportException(
"Decoded JSON is not an array or object. Data: " . substr($trimmedData, 0, 200) . "...",
JsonRpcMessage::INVALID_REQUEST
);
}

$messages = [];

if (empty($decodedData)) { // Handles `[]` case, which is a valid empty batch.
return [];
}

// Determine if it's a batch (indexed array) or single message (associative array)
$isBatch = array_keys($decodedData) === range(0, count($decodedData) - 1);

if ($isBatch) {
// It's an array of messages (batch)
if (empty($decodedData)) { // Already handled above, but as a safeguard for logic.

Check failure on line 102 in src/Transport/AbstractTransport.php

View workflow job for this annotation

GitHub Actions / test 

Variable $decodedData in empty() always exists and is not falsy.
return [];

Check warning on line 103 in src/Transport/AbstractTransport.php

View check run for this annotation

Codecov / codecov/patch

src/Transport/AbstractTransport.php#L103 

Added line #L103 was not covered by tests
}
foreach ($decodedData as $index => $messageData) {
if (!is_array($messageData)) {
throw new TransportException(
"Invalid item in batch at index {$index}: not an object/array. Item: " . substr((string)json_encode($messageData), 0, 100),
JsonRpcMessage::INVALID_REQUEST
);
}
try {
$messageJson = json_encode($messageData);
if ($messageJson === false) {
// This case should be rare if $messageData is a valid array from json_decode
throw new TransportException(
"Failed to re-encode message in batch at index {$index}. Data: " . substr((string)json_encode($messageData), 0, 100) . "...",
JsonRpcMessage::INVALID_REQUEST // Or PARSE_ERROR, though it's an encoding issue here
);

Check warning on line 119 in src/Transport/AbstractTransport.php

View check run for this annotation

Codecov / codecov/patch

src/Transport/AbstractTransport.php#L116-L119 

Added lines #L116 - L119 were not covered by tests
}
$messages[] = JsonRpcMessage::fromJson($messageJson);
} catch (\RuntimeException $e) {
// Assuming JsonRpcMessage::fromJson throws RuntimeException with appropriate codes
throw new TransportException(
"Error parsing message in batch at index {$index}: " . $e->getMessage() . ". Data: " . substr((string)json_encode($messageData), 0, 100) . "...",
$e->getCode() !== 0 ? $e->getCode() : JsonRpcMessage::INVALID_REQUEST, // Use exception code if available
$e
);
} catch (\Exception $e) { // Catch any other unexpected errors during message construction
throw new TransportException(
"Unexpected error parsing message in batch at index {$index}: " . $e->getMessage() . ". Data: " . substr((string)json_encode($messageData), 0, 100) . "...",
JsonRpcMessage::INTERNAL_ERROR,
$e
);

Check warning on line 134 in src/Transport/AbstractTransport.php

View check run for this annotation

Codecov / codecov/patch

src/Transport/AbstractTransport.php#L129-L134 

Added lines #L129 - L134 were not covered by tests
}
}
} else {
// It's a single message (associative array)
// Use $trimmedData directly as it's the original JSON string for the single message.
try {
$messages[] = JsonRpcMessage::fromJson($trimmedData);
} catch (\RuntimeException $e) {
// Assuming JsonRpcMessage::fromJson throws RuntimeException with appropriate codes
throw new TransportException(
"Error parsing single message: " . $e->getMessage() . ". Data: " . substr($trimmedData, 0, 200) . "...",
$e->getCode() !== 0 ? $e->getCode() : JsonRpcMessage::INVALID_REQUEST, // Use exception code if available
$e
);
} catch (\Exception $e) { // Catch any other unexpected errors
throw new TransportException(
"Unexpected error parsing single message: " . $e->getMessage() . ". Data: " . substr($trimmedData, 0, 200) . "...",
JsonRpcMessage::INTERNAL_ERROR,
$e
);

Check warning on line 154 in src/Transport/AbstractTransport.php

View check run for this annotation

Codecov / codecov/patch

src/Transport/AbstractTransport.php#L149-L154 

Added lines #L149 - L154 were not covered by tests
}
}

return $messages;
}

/**
* Decodes and validates a received JSON string into a JsonRpcMessage.
*
* @deprecated This method is deprecated in favor of `parseMessages()`, which can handle
* single messages as well as batches and provides more robust error handling
* for structured data. Consider using `parseMessages()` and then extracting
* the first message if only one is expected.
* @param string $data The raw JSON data received from the transport.
* @return JsonRpcMessage|null The decoded message, or null if decoding fails (e.g., invalid JSON).
* @throws TransportException If the received data string exceeds MAX_MESSAGE_SIZE before decoding.
Expand Down
Loading
Loading