feat(ai): stream AI suggestions over HTTP - #664
Conversation
2265cba to
6bf913f
Compare
2ee7bd0 to
497be8b
Compare
497be8b to
9ff73a0
Compare
9ff73a0 to
8f8ee4a
Compare
8f8ee4a to
bd3667c
Compare
| }); | ||
| } | ||
|
|
||
| const res: any = new Writable({ |
There was a problem hiding this comment.
probably could be typed, since you assign it right away
There was a problem hiding this comment.
Added FakeResponse for this.
| const response = result.toUIMessageStreamResponse(); | ||
|
|
||
| res.status(response.status); | ||
| response.headers.forEach((value, key) => res.setHeader(key, value)); | ||
|
|
||
| if (!response.body) { | ||
| res.end(); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| Readable.fromWeb(response.body as NodeReadableStream<Uint8Array>).pipe(res); | ||
| } catch (error) { | ||
| next(error); | ||
| } | ||
| }); |
There was a problem hiding this comment.
we can just use
result.pipeUIMessageStreamToResponse(res);with extra options or just
result.pipeTextStreamToResponse(res);since we don't need any metadata toolcalls etc
There was a problem hiding this comment.
Good suggestion. And since I don't see any possibilities that ask-ai will need text/event-stream format, I'm using pipeTextStreamToResponse here.
@FeironoX5 If you're still working on stream receiving in hawk.garage, could you agree/disagree with this?
There was a problem hiding this comment.
Correction to my earlier reply: text/event-stream may turn out to be needed after all. On a rejected answer the guard in #668 can only append the fallback as more text, so the client shows a truncated prefix glued to it. Signalling a failure on its own channel needs the UI message stream. Leaving pipeTextStreamToResponse for now and revisiting once the feature is testable on stage.
bd3667c to
97fa6c2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/ai-prompt-injection #664 +/- ##
==========================================================
Coverage ? 48.91%
==========================================================
Files ? 62
Lines ? 2819
Branches ? 638
==========================================================
Hits ? 1379
Misses ? 1362
Partials ? 78 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
createFakeResponse assigned every Express-shaped method it needed right after construction, so it could be typed as that shape from the start instead of any - reviewer feedback on #664. Also adds writeHead, which the AI SDK's response-piping helpers call directly, bypassing Express's status()/setHeader() convenience methods.
routes.ts landed in integrations/vercel-ai/ in the original commit, even though it only calls askAiService and never touches the transport - the same domain-code-in-an-adapter-directory problem services/ai.ts itself had before it moved into askAi/. Wire its imports to the new location and expose it through the askAi barrel, alongside AskAiService. Also switches result.toUIMessageStreamResponse() + manual Response-to-Express bridging for result.pipeTextStreamToResponse(res) - reviewer feedback on #664. The model call is tool-less by design (see VercelAIApi's docstring), so there's no tool-call/reasoning metadata to carry, and plain text drops the SSE envelope this otherwise never needed. Drops the now-unused ReadableStream/Response ESLint globals that only existed for the old SSE-based test fixture.
There was a problem hiding this comment.
Pull request overview
Adds an HTTP streaming endpoint for AI suggestions and wires it into the API, alongside extending the Vercel AI integration with a streaming call and updating tests/utilities to support streaming responses.
Changes:
- Introduces
GET /integration/ai/streamExpress route and app wiring for AI suggestion streaming. - Extends the Vercel AI integration with a
stream()method and adds service-levelstreamSuggestion(). - Adds/updates Jest tests and introduces a reusable Express request/response test helper that can capture streamed bodies/headers.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/services/askAiRoutes.test.ts | New tests covering auth/validation/error cases and streaming response behavior for /integration/ai/stream. |
| test/services/askAi.test.ts | Adds service-level coverage for streamSuggestion() behavior. |
| test/integrations/vercel-ai.test.ts | Adds integration-level coverage for vercelAIApi.stream() forwarding to streamText. |
| test/integrations/github-routes.test.ts | Refactors tests to reuse the new makeExpressRequest helper. |
| test/helpers/expressRequest.ts | New helper to drive Express apps without a socket and capture streamed responses/headers. |
| src/services/types.ts | Exports Event type for reuse by services. |
| src/services/askAi/service.ts | Adds streamSuggestion() and refactors event lookup into getEventOrThrow(). |
| src/services/askAi/routes.ts | New Express router for AI streaming endpoint and authorization checks. |
| src/services/askAi/index.ts | Exports appendAiAssistantRoutes for app integration. |
| src/integrations/vercel-ai/index.ts | Adds stream() wrapper around streamText and centralizes provider gateway options. |
| src/index.ts | Registers AI assistant routes on the main Express app. |
| src/directives/requireUserInWorkspace.ts | Exports checkUserInWorkspaceByProjectId for use from Express routes. |
| package.json | Bumps package version. |
Suppressed comments (2)
src/services/askAi/routes.ts:87
- The inner
catchconverts anystreamSuggestionerror into a 404 and returnserror.messageto the caller. That will misreport transport/DB failures as "not found" and can leak internal error details (e.g. events factory errors that include ids). Only map the known not-found case to 404; rethrow unexpected errors so the outer handler cannext(error)and return a 5xx.
try {
result = await askAiService.streamSuggestion(eventsFactory, eventId, originalEventId);
} catch (error) {
res.status(404).json({ error: error instanceof Error ? error.message : 'Event not found' });
src/services/askAi/routes.ts:91
- This route calls
result.pipeTextStreamToResponse(res), but the PR description says the AI SDK'stoUIMessageStreamResponse()(a Fetch APIResponse) is adapted onto the Express response. As written, there is no adaptation and the call isn’t type-checked (becauseresultis implicitlyany), so a wrong method name or incompatible stream type would only fail at runtime. Consider explicitly usingtoUIMessageStreamResponse()and piping its status/headers/body into Express.
result.pipeTextStreamToResponse(res);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review on #664: projectId comes from req.query, which Express parses as string[] for a repeated key (?projectId=a&projectId=b). The route cast it straight to string and forwarded it to checkUserInWorkspaceByProjectId/getEventsFactory, both expecting a single id - eventId and originalEventId already had the typeof guard this was missing. authorizeProjectAccess now validates and returns the narrowed id instead of the caller re-casting it. makeExpressRequest's query param takes string | string[] now, to let tests simulate a repeated key.
Copilot review on #664: getEventOrThrow only handled a falsy return from getEventRepetition, but it can also throw - EventsFactory throws "Cant find event repetition for repetitionId: ..." on an unmatched id, echoing the raw id back, and an invalid id format throws a raw BSON error. Both reached the HTTP route's catch block unfiltered. Catches and normalizes to the same generic message as the missing-event case.
815fba8 to
c805ff1
Compare
f77e029 to
e796dd5
Compare
e796dd5 to
eb03e95
Compare
eb03e95 to
4e18054
Compare
| * @param {CompletionParams} params - system instruction and prompt to complete | ||
| * @returns {StreamTextResult} text generated by the model, as a stream | ||
| */ | ||
| public stream({ system, prompt }: CompletionParams): ReturnType<typeof streamText> { |
There was a problem hiding this comment.
Vercel implementation should not export Vercel's type outside of itself. Other layers above should not depend on "ai" package.
Maybe we should create own type for streaming text so we can add more integrations later?
There was a problem hiding this comment.
stream() and streamSuggestion() return AiStream now, declared in src/services/askAi/stream.ts. Converting SDK parts into our own types.
| eventsFactory: EventsFactoryInterface, | ||
| eventId: string, | ||
| originalEventId: string | ||
| ): Promise<ReturnType<typeof vercelAIApi.stream>> { |
There was a problem hiding this comment.
Same change, answered in the thread on vercel-ai/index.ts.
There was a problem hiding this comment.
todo: use types form hawk.types
| /** | ||
| * Verify the requesting user is a member of the project's workspace. | ||
| * | ||
| * @param req - Express request | ||
| * @param res - Express response | ||
| * @param projectId - project id from query parameters (may be string[] if repeated) | ||
| * @returns user id and validated project id if authorized, {@code null} otherwise (response already sent) | ||
| */ | ||
| async function authorizeProjectAccess( | ||
| req: express.Request, | ||
| res: express.Response, |
There was a problem hiding this comment.
may be move out to utils with validateProjectAdminAccess?
There was a problem hiding this comment.
I guess it's not worth it to put domain router logic into utils.
New HTTP route GET /integration/ai/stream added. This route calls Ask AI service about specified event and responds text/event-stream. Reponse carrying AiStream which represent AiStreamPart sequence: either text-delta (text-increments generated by AI assistant) or error (failure description during response generation). NOTE: Response doesn't carry reasoning, tooling and start/end parts since they're not required yet. Route checks workspace membership before calling Ask AI. For this purpose function checkUserInWorkspaceByProjectId became exported. Failed membership check leads to response with 403. Also route checks if specified event exists. Failed check leads to response with 404. Aborting request cancel Ask AI suggestion generation. For this purpose AbortController is declared as an eslint global: it is on globalThis since Node 15, but eslint's node env predates it.
913f097 to
0a7df48
Compare
The suggestion reaches the client only once the model has finished writing it, so nothing appears until the whole generation is done.
GET /integration/ai/stream?projectId&eventId&originalEventIdstreams it as server-sent events:Text and failure travel on channels of their own, which
text/plaincannot offer: a body without framing is content by definition, so the server has no way to say that what follows is a failure rather than more of the answer. The route is an Express one and not a GraphQL field because the answer is written onto the response as it is produced; the field serving the one-shot answer is untouched.The adapter translates the AI SDK's stream parts into a
SuggestionStream, anAsyncIterable<SuggestionPart>declared insrc/services/askAi/suggestionStream.ts, and the route writes the events itself. Nothing abovesrc/integrations/vercel-ai/names anything from theaipackage, so a second provider implementsstream(): SuggestionStreamand stops there.SuggestionPartcarries the names the client already reads, so no second vocabulary sits between the adapter and the browser. What the frames no longer carry isid,text-start/text-endand[DONE], none of which Garage reads; streaming reasoning or tool calls later brings block identity back.An
AbortControllertied to the response stops the model when the client goes away, since the gateway otherwise bills for the rest of an answer nobody will read. It is declared as an eslint global because it is onglobalThissince Node 15 while eslint'snodeenv predates it.checkUserInWorkspaceByProjectIdbecomes exported so the route runs the same workspace membership check that guards the GraphQL field. The Express request helper written for the GitHub route tests moves totest/helpers/expressRequest.ts, which is whygithub-routes.test.tsloses a hundred lines that have nothing to do with streaming.Event lookup now fails the same way whether the factory throws or returns nothing: both become
Event not found, which the route answers with 404. The one-shot path shares that helper, so a lookup error no longer leaves it carrying its original message.