-
Notifications
You must be signed in to change notification settings - Fork 79
fix: error and input handling #130
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.
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
Conversation
MQ37
commented
May 30, 2025
- fixes tool error and input handling
- fixed test suite inconsistencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves input validation and error handling across various tools and fixes inconsistencies in the integration test suite.
- Re-enabled and corrected an integration test, updated client usage, and refined reset logic.
- Added guards for required inputs and API error handling in multiple tool commands.
- Disabled default actors in test servers to align with new enableDefaultActors behavior.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/suite.ts | Re-enabled a skipped test and updated client/reset interactions |
| tests/integration/actor.server-streamable.test.ts | Added enableDefaultActors: false for streamable server |
| tests/integration/actor.server-sse.test.ts | Added enableDefaultActors: false for SSE server |
| src/tools/store_collection.ts | Validated non-empty search strings |
| src/tools/run_collection.ts | Wrapped runs list call in try/catch for API errors |
| src/tools/run.ts | Added runId validation and API error handling |
| src/tools/key_value_store.ts | Validated storeId and recordKey inputs |
| src/tools/helpers.ts | Added validation and error handling in addTool/removeTool |
| src/tools/dataset.ts | Added datasetId validation and wrapped calls in try/catch |
| src/tools/build.ts | Added actorName validation and null checks in build tool |
| src/tools/actor.ts | Added actorId validation and wrapped calls in try/catch |
Comments suppressed due to low confidence (1)
src/tools/run_collection.ts:1
- The
Ajvimport is unused in this file; it can be removed to clean up unused dependencies.
import { Ajv } from 'ajv';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses error and input handling issues across various tools and test files while also resolving inconsistencies in the test suite.
- Updated tests to adjust server and client handling, including re-enabling a previously disabled test
- Added input validations with clear error messages across run, dataset, key-value store, actor, and build tools
- Improved error logging in the MCP server to handle Apify API errors with detailed messages
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/suite.ts | Adjusted tests for default tools and server reset behavior |
| tests/integration/actor.server-streamable.test.ts | Updated server instantiation to disable default actors |
| tests/integration/actor.server-sse.test.ts | Updated server instantiation to disable default actors |
| src/tools/store_collection.ts | Added validation to prevent empty search strings |
| src/tools/run.ts | Introduced run ID validation and improved error responses |
| src/tools/key_value_store.ts | Added validations for storeId and record key |
| src/tools/helpers.ts | Enforced non-empty actorName check in add and remove tool functions |
| src/tools/dataset.ts | Introduced datasetId validation and error handling for missing datasets |
| src/tools/build.ts | Added validation to ensure actorName is provided and exists |
| src/tools/actor.ts | Ensured actorId validation and proper error response when actor is not found |
| src/mcp/server.ts | Enhanced error logging for Apify API errors with detailed messages |
Comments suppressed due to low confidence (1)
tests/integration/suite.ts:106
- The previously disabled test for listing default tools has been re-enabled. Please confirm that the underlying server reset issue (which previously loaded default Actors) has been resolved to avoid unexpected test failures.
it('should list all default tools and two loaded Actors', async () => {
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes input validation and error handling across tool commands and streamlines integration test behavior.
- Strengthen argument schemas with minimum length checks and uniform not-found responses in tools.
- Enhance MCP server to catch Apify API errors and report them gracefully.
- Update integration tests to reflect new default-actor handling and improve server reset flow.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/suite.ts | Re-enabled test, updated client naming and reset logic |
| tests/integration/actor.server-streamable.test.ts | Added enableDefaultActors: false to server setup |
| tests/integration/actor.server-sse.test.ts | Added enableDefaultActors: false to SSE server setup |
| src/tools/run.ts | Added .min(1) for runId and uniform not-found check |
| src/tools/key_value_store.ts | Added .min(1) validations for store and record IDs |
| src/tools/helpers.ts | Enhanced add/remove tool validation and existence checks |
| src/tools/dataset.ts | Added .min(1) validations and not-found responses |
| src/tools/build.ts | Added .min(1) validation and not-found response for definitions |
| src/tools/actor.ts | Updated imports, added .min(1) and not-found handler for actor |
| src/mcp/server.ts | Caught ApifyApiError to return formatted error messages |
Comments suppressed due to low confidence (5)
tests/integration/suite.ts:212
- [nitpick] Variable names like
firstClientandsecondClientcan be more descriptive (e.g.,initialClient/resetClient) to clarify their roles in the test.
const firstClient = await createClientFn({ enableAddingActors: true });
tests/integration/suite.ts:217
- The commented-out TODOs and dead code around server reset should be removed or refactored now that the new reset flow is implemented.
// TODO: The reset functions sets the enableAddingActors to false, which is not expected
tests/integration/actor.server-streamable.test.ts:26
- Consider adding assertions to verify that default actors are indeed disabled when
enableDefaultActorsis set tofalse.
mcpServer = new ActorsMcpServer({ enableAddingActors: false, enableDefaultActors: false });
tests/integration/actor.server-sse.test.ts:24
- Similarly, add tests to confirm that SSE streams do not include default actors when
enableDefaultActorsis disabled.
mcpServer = new ActorsMcpServer({ enableAddingActors: false, enableDefaultActors: false });
src/tools/helpers.ts:127
apifyMcpServer.toolsmay not exist or reflect the same collection aslistAllToolNames(). Consider usinglistAllToolNames()or verifying the correct property for existence checks.
if (!apifyMcpServer.tools.has(parsed.toolName)) {
jirispilka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, only one nit (questions)