-
-
Notifications
You must be signed in to change notification settings - Fork 456
Feature/Elysia server integration #898
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
Feature/Elysia server integration #898
Conversation
🦋 Changeset detectedLatest commit: 1a54a34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
16 issues found across 46 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/server-elysia/README.md">
<violation number="1" location="packages/server-elysia/README.md:241">
P2: GitHub Repository link points to a personal fork (`MGrin/voltagent`) instead of the official repository (`VoltAgent/voltagent`). This could confuse users looking for the main project.</violation>
</file>
<file name="packages/server-elysia/src/mcp/elysia-sse-bridge.ts">
<violation number="1" location="packages/server-elysia/src/mcp/elysia-sse-bridge.ts:53">
P1: Inconsistent error handling: `void listener()` doesn't handle errors, unlike `triggerAbortListeners()` which uses try/catch. This can cause unhandled promise rejections if the listener throws or rejects.</violation>
</file>
<file name="packages/server-elysia/src/auth/middleware.ts">
<violation number="1" location="packages/server-elysia/src/auth/middleware.ts:215">
P1: The overridden `request.json()` method will fail on subsequent calls because the body stream can only be read once. The result should be cached after the first read to support multiple invocations.</violation>
</file>
<file name="packages/server-elysia/src/routes/a2a.routes.ts">
<violation number="1" location="packages/server-elysia/src/routes/a2a.routes.ts:163">
P2: All errors return HTTP 400, but server errors (internal failures, database issues) should return 500. The established pattern in this codebase (see `agent.routes.ts`) uses `httpStatus || 500` to properly differentiate. Consider checking `response.error?.code` for internal error codes (-32603, -32000 to -32099) to return 500 for server errors.</violation>
<violation number="2" location="packages/server-elysia/src/routes/a2a.routes.ts:193">
P2: Error handling only considers 404 or 400 status codes, missing 500 for server errors. Internal error code (-32603) and server-defined error codes (-32000 to -32099) should return HTTP 500.</violation>
</file>
<file name="packages/server-elysia/src/routes/tool.routes.ts">
<violation number="1" location="packages/server-elysia/src/routes/tool.routes.ts:43">
P1: Error handling discards the error details and HTTP status code from `handleListTools`. The response includes `error` message and `httpStatus` that should be returned to the client. Consider using Elysia's `set.status` and returning the response object instead of throwing a generic error.</violation>
<violation number="2" location="packages/server-elysia/src/routes/tool.routes.ts:67">
P1: Error handling discards the error details and HTTP status code from `handleExecuteTool`. When a tool is not found, the handler returns `httpStatus: 404` with a specific error message, but this throws a generic 500 error instead. Consider using Elysia's `set.status` and returning the response object.</violation>
</file>
<file name="packages/server-elysia/THIRD_PARTY_NOTICES.md">
<violation number="1" location="packages/server-elysia/THIRD_PARTY_NOTICES.md:9">
P2: Incorrect copyright attribution for `zod-to-json-schema`. The repository URL shows the author is Stefan Terdell (StefanTerdell), not Saltyaom. This appears to be a copy-paste error from the Elysia entries.</violation>
</file>
<file name="packages/server-elysia/src/utils/custom-endpoints.ts">
<violation number="1" location="packages/server-elysia/src/utils/custom-endpoints.ts:218">
P2: Mutating `operation.tags` in place modifies the original `baseDoc` object because the spread operator only creates a shallow copy of `paths`. Consider creating a new tags array instead of pushing to the existing one to avoid unexpected side effects.</violation>
</file>
<file name="packages/server-elysia/src/elysia-server-provider.spec.ts">
<violation number="1" location="packages/server-elysia/src/elysia-server-provider.spec.ts:72">
P2: Test has no assertions. The `createWebSocketServer` mock is available - add an assertion to verify it was called when `enableWebSocket: true` is set.</violation>
<violation number="2" location="packages/server-elysia/src/elysia-server-provider.spec.ts:78">
P2: Test has no assertions. The mock logger is available in `mockDeps.logger` - add an assertion to verify logging behavior or another observable effect.</violation>
<violation number="3" location="packages/server-elysia/src/elysia-server-provider.spec.ts:92">
P2: Modifying shared `mockApp.routes` can cause test pollution. `vi.clearAllMocks()` does not reset property assignments. Consider resetting `mockApp.routes = []` in `afterEach` or using a local mock for this test.</violation>
</file>
<file name="packages/server-elysia/src/routes/agent.routes.ts">
<violation number="1" location="packages/server-elysia/src/routes/agent.routes.ts:211">
P2: `Number.parseInt` returns `NaN` for non-numeric strings. Add a fallback and ensure the limit is at least 1 to prevent invalid pagination.</violation>
</file>
<file name="packages/server-elysia/src/routes/observability.spec.ts">
<violation number="1" location="packages/server-elysia/src/routes/observability.spec.ts:25">
P2: Inconsistent mock: `OBSERVABILITY_ROUTES` uses the actual import but `OBSERVABILITY_MEMORY_ROUTES` is hardcoded. Use `actual.OBSERVABILITY_MEMORY_ROUTES` for consistency and to prevent drift if routes change in server-core.</violation>
</file>
<file name="packages/server-elysia/src/routes/mcp.routes.ts">
<violation number="1" location="packages/server-elysia/src/routes/mcp.routes.ts:264">
P2: `JSON.parse()` on user-provided query parameter should be wrapped in try-catch to handle malformed JSON gracefully. Invalid JSON will cause an unhandled `SyntaxError`.</violation>
</file>
<file name="packages/server-elysia/src/routes/update.routes.ts">
<violation number="1" location="packages/server-elysia/src/routes/update.routes.ts:87">
P2: Inconsistent route path usage - other routes use `UPDATE_ROUTES.checkUpdates.path` and `UPDATE_ROUTES.installUpdates.path`, but this route uses a hardcoded string. Use `UPDATE_ROUTES.installSingleUpdate.path` for consistency and maintainability.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
0e5e397 to
5232c88
Compare
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.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".changeset/full-crews-battle.md">
<violation number="1" location=".changeset/full-crews-battle.md:10">
P2: Placeholder text `- TO BE REMOVED` will appear in release notes. This should be removed before merging as it will be included in the published changelog.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
f67552a to
5232c88
Compare
omeraplak
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.
Hey, it looks great! Would you like to add the docs under
https://voltagent.dev/docs/api/overview/ ?
Here’s the file:
https://github.com/VoltAgent/voltagent/blob/main/website/docs/api/overview.md
If you prefer, I can also add them after the merge.
Yep, will add it in there in a few, thanks! |
…ndling - Fix documentation links and repository references in README and notices - Implement body caching in auth middleware to support multiple JSON reads - Improve error status code mapping for A2A and Tool routes - Fix mutation bugs in OpenAPI document generation - Correct pagination logic and query parsing in agent routes - Fix SSE bridge abort handling and MCP argument parsing - Update tests to match constructor signature and improve mocks
5232c88 to
a2c32e0
Compare
- Updated overview.md - Update authentication.md - Updated server-architecture.md
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.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="website/docs/api/server-architecture.md">
<violation number="1" location="website/docs/api/server-architecture.md:262">
P2: The types `RouteRegister` and `MiddlewareRegister` don't exist in the codebase. The actual implementation uses `Record<string, () => void>` for both `routes` and `middlewares`. This documentation inaccuracy could confuse users trying to understand the API.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
|
Thank you so much @MGrin ⚡️ |
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
There is no ElysiaJS server integration available for VoltAgent.
What is the new behavior?
This PR introduces
@voltagent/server-elysia, a new high-performance server provider built on top of ElysiaJS.Key features:
listen)fixes (issue)
Notes for reviewers
packages/server-elysia/example.Summary by cubic
Introduces @voltagent/server-elysia, a high-performance ElysiaJS server provider with full feature parity to server-hono (agents, workflows, MCP, A2A) and built-in OpenAPI docs. Includes an example app and extensive tests.
New Features
Bug Fixes
Written for commit 1a54a34. Summary will update automatically on new commits.