Ocean/pipeline integration tests d552 - #21
Closed
Ocean82 wants to merge 2 commits into
Closed
Conversation
Parse server meta through SSE/complete responses into ChatMessage, show provider details in DEV chat UI, and label BYOK by actual provider/host instead of hardcoding openrouter. Co-authored-by: Ocean82 <Ocean82@users.noreply.github.com>
Replace brainDispatcher/processMessage mocks with deterministic-dispatcher and llm-gateway stages, mocking agentClient, intent/mode, and analysis deps. Co-authored-by: Ocean82 <Ocean82@users.noreply.github.com>
Reviewer's GuideThis PR refactors the AI chat routing pipeline to split deterministic spreadsheet skills from LLM handling, wires provider identity metadata end-to-end from server to UI, and updates integration/smoke tests plus deterministic dispatcher to operate on resolved analysis targets instead of test-only stubs. Sequence diagram for providerMeta propagation from server to UIsequenceDiagram
actor User
participant ChatPanel
participant ChatService as processChatMessage
participant Pipeline as LLMGatewayStage
participant Server as runLlmChat
User->>ChatPanel: submit message
ChatPanel->>ChatService: processChatMessage(input)
ChatService->>Pipeline: router.process(pipelineContext)
Pipeline->>Server: runLlmChat(params)
Server-->>Pipeline: ServerChatResponse{ meta }
Pipeline-->>ChatService: StageResult{ metadata.providerMeta }
ChatService-->>ChatPanel: ChatMessage{ providerMeta }
ChatPanel-->>User: render providerMeta in dev details
Flow diagram for updated chat pipeline stagesflowchart LR
subgraph client_pipeline[Client chat pipeline]
AP[AgentParserStage]
TR[TemplateResolverStage]
IC[IntentClassifierStage]
DD[DeterministicDispatcherStage]
LG[LLMGatewayStage]
AP --> TR --> IC --> DD --> LG
end
U[User input] --> AP
LG --> S[Server runLlmChat]
S --> LG
LG --> R[Final assistant reply]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThe change separates deterministic and LLM pipeline stages, tracks BYOK and server-provider metadata, validates complete SSE events, propagates provider metadata through chat results, and displays it in development builds. Integration tests now cover the new routing and fallback behavior. ChangesLLM pipeline and provider metadata
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ChatService
participant DeterministicDispatcher
participant LLMGateway
participant AgentClient
participant AIServer
ChatService->>DeterministicDispatcher: process classified request
DeterministicDispatcher-->>ChatService: claim or pass
ChatService->>LLMGateway: process unclaimed request
LLMGateway->>AgentClient: stream chat request
AgentClient->>AIServer: send request
AIServer-->>AgentClient: complete SSE with provider metadata
AgentClient-->>LLMGateway: parsed server response
LLMGateway-->>ChatService: result with providerMeta
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
defaultIntent()helper is duplicated in bothpipeline.integration.test.tsandsmokeTest.integration.test.ts; consider extracting this to a shared test utility to avoid divergence and keep future intent shape changes in one place. - The large block of Vitest mocks for context/analysis (buildContext, analysisTarget, sheetProfile, budget analysis, etc.) is now duplicated across the two integration test files; factoring these into a shared test setup would reduce maintenance overhead and help keep the mocked behavior consistent.
- In
chatWithAgentServerStream, each SSE event JSON is parsed twice (once inline for token events and again inparseCompleteSseEvent); you could pass an already-parsed object into a helper or branch ontypein a single parse to avoid redundant JSON parsing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `defaultIntent()` helper is duplicated in both `pipeline.integration.test.ts` and `smokeTest.integration.test.ts`; consider extracting this to a shared test utility to avoid divergence and keep future intent shape changes in one place.
- The large block of Vitest mocks for context/analysis (buildContext, analysisTarget, sheetProfile, budget analysis, etc.) is now duplicated across the two integration test files; factoring these into a shared test setup would reduce maintenance overhead and help keep the mocked behavior consistent.
- In `chatWithAgentServerStream`, each SSE event JSON is parsed twice (once inline for token events and again in `parseCompleteSseEvent`); you could pass an already-parsed object into a helper or branch on `type` in a single parse to avoid redundant JSON parsing.
## Individual Comments
### Comment 1
<location path="src/ai/agentClient.ts" line_range="25-34" />
<code_context>
+}
+
+/** Parse an SSE `data:` JSON payload into a ServerChatResponse when type=complete. */
+export function parseCompleteSseEvent(jsonStr: string): ServerChatResponse | null {
+ try {
+ const event = JSON.parse(jsonStr) as {
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid double JSON.parse for SSE events to reduce overhead and inconsistency risk.
Within the stream loop, `jsonStr` is parsed twice: once to inspect `type`, then again in `parseCompleteSseEvent`. This adds per-event overhead and creates two separate parsing paths (a minimal `{ type?: string; content?: string }` shape vs. the full event type). Refactor so the event is parsed only once—either by passing the already-parsed object into `parseCompleteSseEvent`, or by making `parseCompleteSseEvent` the single parsing entry point for SSE chunks.
Suggested implementation:
```typescript
/** Parse a parsed SSE `data:` JSON payload into a ServerChatResponse when type=complete. */
export function parseCompleteSseEvent(event: {
type?: string
message?: string
actions?: ServerAgentAction[]
source?: string
reasoning?: string
suggestions?: string[]
meta?: ProviderMeta
}): ServerChatResponse | null {
try {
if (event.type !== 'complete' || typeof event.message !== 'string') return null
return {
```
To fully avoid double JSON.parse:
1. In the SSE stream loop where `jsonStr` is read, keep a single `const event = JSON.parse(jsonStr)` and pass `event` into `parseCompleteSseEvent(event)` instead of `parseCompleteSseEvent(jsonStr)`.
2. Remove any other callers that still pass a string and ensure they now pass the already-parsed event object.
3. If there is a separate lightweight parse for `{ type?: string; content?: string }`, you can reuse that parsed object by widening its type (or re-parsing once into the richer shape), then forwarding it into `parseCompleteSseEvent`.
</issue_to_address>
### Comment 2
<location path="server/src/index.ts" line_range="241-244" />
<code_context>
+ } catch {
+ // keep custom
+ }
+ providerMeta = {
+ provider: byok.provider?.trim() || byokHost || 'byok',
+ model: byok.model?.trim() || byokHost,
+ }
+ byokSucceeded = true
</code_context>
<issue_to_address>
**suggestion:** Defaulting BYOK model to the host name may be misleading or confusing.
Here `providerMeta.model` falls back to `byokHost` when `byok.model` is empty, so the "model" field may show a hostname (e.g. `api.openai.com`) instead of a model identifier. To avoid confusion when debugging or inspecting replies, consider using an explicit placeholder (e.g. `'unknown-model'` or `'custom'`) or omitting the model rather than reusing the host value.
```suggestion
providerMeta = {
provider: byok.provider?.trim() || byokHost || 'byok',
model: byok.model?.trim() || 'unknown-model',
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+25
to
+34
| export function parseCompleteSseEvent(jsonStr: string): ServerChatResponse | null { | ||
| try { | ||
| const event = JSON.parse(jsonStr) as { | ||
| type?: string | ||
| message?: string | ||
| actions?: ServerAgentAction[] | ||
| source?: string | ||
| reasoning?: string | ||
| suggestions?: string[] | ||
| meta?: ProviderMeta |
There was a problem hiding this comment.
suggestion (performance): Avoid double JSON.parse for SSE events to reduce overhead and inconsistency risk.
Within the stream loop, jsonStr is parsed twice: once to inspect type, then again in parseCompleteSseEvent. This adds per-event overhead and creates two separate parsing paths (a minimal { type?: string; content?: string } shape vs. the full event type). Refactor so the event is parsed only once—either by passing the already-parsed object into parseCompleteSseEvent, or by making parseCompleteSseEvent the single parsing entry point for SSE chunks.
Suggested implementation:
/** Parse a parsed SSE `data:` JSON payload into a ServerChatResponse when type=complete. */
export function parseCompleteSseEvent(event: {
type?: string
message?: string
actions?: ServerAgentAction[]
source?: string
reasoning?: string
suggestions?: string[]
meta?: ProviderMeta
}): ServerChatResponse | null {
try {
if (event.type !== 'complete' || typeof event.message !== 'string') return null
return {To fully avoid double JSON.parse:
- In the SSE stream loop where
jsonStris read, keep a singleconst event = JSON.parse(jsonStr)and passeventintoparseCompleteSseEvent(event)instead ofparseCompleteSseEvent(jsonStr). - Remove any other callers that still pass a string and ensure they now pass the already-parsed event object.
- If there is a separate lightweight parse for
{ type?: string; content?: string }, you can reuse that parsed object by widening its type (or re-parsing once into the richer shape), then forwarding it intoparseCompleteSseEvent.
Comment on lines
+241
to
+244
| providerMeta = { | ||
| provider: byok.provider?.trim() || byokHost || 'byok', | ||
| model: byok.model?.trim() || byokHost, | ||
| } |
There was a problem hiding this comment.
suggestion: Defaulting BYOK model to the host name may be misleading or confusing.
Here providerMeta.model falls back to byokHost when byok.model is empty, so the "model" field may show a hostname (e.g. api.openai.com) instead of a model identifier. To avoid confusion when debugging or inspecting replies, consider using an explicit placeholder (e.g. 'unknown-model' or 'custom') or omitting the model rather than reusing the host value.
Suggested change
| providerMeta = { | |
| provider: byok.provider?.trim() || byokHost || 'byok', | |
| model: byok.model?.trim() || byokHost, | |
| } | |
| providerMeta = { | |
| provider: byok.provider?.trim() || byokHost || 'byok', | |
| model: byok.model?.trim() || 'unknown-model', | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ai/agentClient.ts`:
- Around line 39-43: Validate optional completion fields in the event-mapping
logic of agentClient: ensure source matches the allowed enum, suggestions is an
array of strings, and meta contains valid ProviderMeta string fields before
forwarding them. Reject or normalize malformed values so ChatPanel receives only
valid data, and add tests covering each malformed field.
- Around line 136-142: Update the streaming read loop around JSON.parse,
parseCompleteSseEvent, and onToken to retain an undecoded-line buffer across
chunks. Process only newline-terminated SSE frames, append each decoded chunk to
the buffer, and after the stream ends flush the decoder and process any
remaining buffered line so token and complete events are preserved.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50ff659b-0f11-4f6f-82b3-4f560801205e
📒 Files selected for processing (15)
server/src/index.tssrc/ai/agentClient.test.tssrc/ai/agentClient.tssrc/ai/brain.tssrc/ai/pipeline/__tests__/pipeline.integration.test.tssrc/ai/pipeline/__tests__/smokeTest.integration.test.tssrc/ai/pipeline/index.tssrc/ai/pipeline/stages/deterministicDispatcher.tssrc/ai/pipeline/stages/index.tssrc/ai/pipeline/stages/llmGateway.tssrc/ai/responseBuilder.tssrc/ai/types.tssrc/components/ChatPanel.tsxsrc/services/chatService.tssrc/types/api.ts
Comment on lines
+39
to
+43
| actions: Array.isArray(event.actions) ? event.actions : [], | ||
| source: (event.source as ServerChatResponse['source']) ?? 'llm', | ||
| reasoning: event.reasoning, | ||
| suggestions: event.suggestions, | ||
| meta: event.meta, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate all optional completion fields before forwarding them.
JSON.parse plus a type assertion does not validate source, suggestions, or meta. For example, a completion event with suggestions: "text" reaches ChatPanel, where the assistant-message renderer calls .map() and throws. Validate the source enum, string-array suggestions, and both ProviderMeta strings. Reject or normalize invalid values before returning the response. Add malformed-field tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ai/agentClient.ts` around lines 39 - 43, Validate optional completion
fields in the event-mapping logic of agentClient: ensure source matches the
allowed enum, suggestions is an array of strings, and meta contains valid
ProviderMeta string fields before forwarding them. Reject or normalize malformed
values so ChatPanel receives only valid data, and add tests covering each
malformed field.
Comment on lines
+136
to
+142
| const parsed = JSON.parse(jsonStr) as { type?: string; content?: string } | ||
| if (parsed.type === 'token' && typeof parsed.content === 'string') { | ||
| onToken(parsed.content) | ||
| continue | ||
| } | ||
| const complete = parseCompleteSseEvent(jsonStr) | ||
| if (complete) finalResponse = complete |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve SSE frames that span stream chunks.
A ReadableStream chunk can end inside a data: line. text.split('\n') discards that partial line, and the next chunk no longer starts with data: . This can drop token events or the only complete event and return null after streamed text. Keep an undecoded-line buffer between reads, process only newline-terminated frames, and flush the decoder and buffer when the stream ends.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ai/agentClient.ts` around lines 136 - 142, Update the streaming read loop
around JSON.parse, parseCompleteSseEvent, and onToken to retain an
undecoded-line buffer across chunks. Process only newline-terminated SSE frames,
append each decoded chunk to the buffer, and after the stream ends flush the
decoder and process any remaining buffered line so token and complete events are
preserved.
There was a problem hiding this comment.
Pull request overview
This PR modernizes the chat/AI routing pipeline by splitting the legacy “BrainDispatcher” responsibilities into a deterministic stage and a server-LLM gateway stage, while also propagating normalized LLM provider/model metadata from server responses through the client and into the dev UI.
Changes:
- Replace BrainDispatcher usage in the chat pipeline with
DeterministicDispatcher(local skills) +LLMGateway(terminal server LLM stage), and update integration/smoke tests accordingly. - Add provider/model metadata plumbing end-to-end: server emits
meta, client parsescompleteSSE events withmeta, and assistantChatMessagecan carryproviderMeta. - Show provider details for assistant messages in dev builds.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/api.ts | Introduces ProviderMeta and attaches it to ChatMessage. |
| src/services/chatService.ts | Updates pipeline stages and maps stage metadata into rendered chat messages. |
| src/components/ChatPanel.tsx | Adds a dev-only <details> UI for provider/model display on assistant messages. |
| src/ai/types.ts | Extends ToolResult to carry provider metadata. |
| src/ai/responseBuilder.ts | Propagates providerMeta when converting ToolResult → ChatMessage. |
| src/ai/pipeline/stages/llmGateway.ts | Emits provider metadata into StageResult.metadata from server responses. |
| src/ai/pipeline/stages/index.ts | Exports deterministic + gateway stages and deprecates brain dispatcher export. |
| src/ai/pipeline/stages/deterministicDispatcher.ts | Resolves analysis targets from PipelineContext when not explicitly provided. |
| src/ai/pipeline/index.ts | Re-exports the new stages from the pipeline module. |
| src/ai/pipeline/tests/smokeTest.integration.test.ts | Updates smoke tests to assert deterministic vs LLM routing with new stages. |
| src/ai/pipeline/tests/pipeline.integration.test.ts | Updates integration routing tests for new stage split and mocking. |
| src/ai/brain.ts | Adds providerMeta to legacy brain path ToolResult (transitional compatibility). |
| src/ai/agentClient.ts | Parses complete SSE events (including meta) and copies metadata into ChatMessage. |
| src/ai/agentClient.test.ts | Adds unit coverage for parsing complete SSE events and metadata propagation. |
| server/src/index.ts | Normalizes provider/model metadata (BYOK vs server providers) and returns it as meta. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| toolUsed: result.metadata?.toolUsed as string | undefined, | ||
| reasoning: result.metadata?.reasoning as string | undefined, | ||
| suggestions: result.suggestions, | ||
| providerMeta: result.metadata?.providerMeta as { provider: string; model: string } | undefined, |
| chartConfig?: ChartConfig | ||
| toolUsed?: string | ||
| reasoning?: string | ||
| providerMeta?: { provider: string; model: string } |
Comment on lines
+36
to
+44
| if (event.type !== 'complete' || typeof event.message !== 'string') return null | ||
| return { | ||
| message: event.message, | ||
| actions: Array.isArray(event.actions) ? event.actions : [], | ||
| source: (event.source as ServerChatResponse['source']) ?? 'llm', | ||
| reasoning: event.reasoning, | ||
| suggestions: event.suggestions, | ||
| meta: event.meta, | ||
| } |
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Replace legacy BrainDispatcher stage with separate DeterministicDispatcher and LLMGateway in the chat pipeline, and propagate LLM provider metadata from server responses through to chat messages and dev UI.
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests