Finish priority residuals: provider meta, pipeline, macros, ONNX Path B, BYOK - #20
Merged
Merged
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>
Contributor
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (47)
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 |
Reviewer's GuideCut over the chat pipeline from BrainDispatcher to separate DeterministicDispatcher + LLMGateway, add a MacroPlanner + real macro execution with undo grouping, wire server-side ONNX Path B for MiniLM, and propagate server LLM provider metadata (including BYOK) end-to-end into dev chat UI while hardening BYOK SSRF validation and usage metering. Sequence diagram for execute_macro macro execution with undo groupingsequenceDiagram
actor User
participant ChatService as processChatMessage
participant Store as executeAction
participant MacroAction as executeMacroAction
participant UndoMgr as createStoreUndoManager
participant StepExec as createToolStepExecutor
participant ExecMacro as executeMacro
User ->> ChatService: input
ChatService ->> Store: AgentAction (tool=execute_macro)
Store ->> MacroAction: executeMacroAction
MacroAction ->> UndoMgr: createStoreUndoManager
MacroAction ->> StepExec: createToolStepExecutor
MacroAction ->> ExecMacro: executeMacro(plan, callbacks, undoManager, stepExecutor)
ExecMacro -->> MacroAction: MacroExecutionResult
MacroAction ->> Store: set undoStack entry
MacroAction -->> Store: ExecutionResult (message, modified)
Store -->> ChatService: ExecutionResult
Flow diagram for updated chat pipeline with MacroPlanner, DeterministicDispatcher, and LLMGatewayflowchart LR
U[User message]
CS[processChatMessage]
PR[PipelineRouter]
AP[createAgentParserStage]
TR[createTemplateResolverStage]
IC[createIntentClassifierStage]
MP[createMacroPlannerStage]
DD[createDeterministicDispatcherStage]
LG[createLLMGatewayStage]
CM[ChatMessage]
U --> CS --> PR
PR --> AP --> TR --> IC --> MP --> DD --> LG
LG -->|ToolResult with providerMeta| CM
DD -->|ToolResult with providerMeta| CM
MP -->|execute_macro actions| CM
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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>
Replace brainDispatcher terminal stage with split stages that consume IntentClassifier enrichment. Remove deprecated brainDispatcher module and update integration/smoke tests. Co-authored-by: Ocean82 <Ocean82@users.noreply.github.com>
Require an explicit stepExecutor (no silent stub in production), add store-backed UndoManager + toolStepExecutor, insert MacroPlanner between IntentClassifier and DeterministicDispatcher, and run execute_macro from Apply as one undoable workbook transaction. Co-authored-by: Ocean82 <Ocean82@users.noreply.github.com>
Wire authenticated /api/onnx with SessionPool path resolution under server/models, add model:copy-deploy script with HF download fallback, and document prod Spreadsheet-RL verify-only plus MiniLM setup. Co-authored-by: Ocean82 <Ocean82@users.noreply.github.com>
Extract shared byokSchema for chat + ai-function, add SSRF regression tests, bump in-memory counters when usage DB writes fail, and document Spreadsheet-RL prod verify + credential rotation in ENV checklist. Co-authored-by: Ocean82 <Ocean82@users.noreply.github.com>
Incomplete intent fixtures broke tsc, and the property test assumed at least two tensor elements which fails for 1x1 shapes.
![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 1 security issue, 4 other issues, and left some high level feedback:
Security issues:
- adm-zip: Denial of Service via crafted ZIP file leading to excessive memory allocation (link)
General comments:
- The new pipeline no longer uses
brain-dispatcher, butstageResultToChatMessagestill branches on that stage name; consider simplifying this logic to only reference the active stages (macro-planner,deterministic-dispatcher,llm-gateway) to avoid dead paths and keep behavior aligned. createMacroPlannerStageaccepts aMacroPlannerDepswithbuildExecContextthat is never used; either remove this dependency from the interface/exports or hook it into clause parsing so the stage’s contract doesn’t include unused parameters.- Both
executeMacroActionandcreateStoreUndoManagerperformstructuredCloneon the workbook, which can be expensive for large documents; consider reusing snapshots or capturing more targeted diffs to reduce cloning overhead during multi-step macro execution and undo operations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new pipeline no longer uses `brain-dispatcher`, but `stageResultToChatMessage` still branches on that stage name; consider simplifying this logic to only reference the active stages (`macro-planner`, `deterministic-dispatcher`, `llm-gateway`) to avoid dead paths and keep behavior aligned.
- `createMacroPlannerStage` accepts a `MacroPlannerDeps` with `buildExecContext` that is never used; either remove this dependency from the interface/exports or hook it into clause parsing so the stage’s contract doesn’t include unused parameters.
- Both `executeMacroAction` and `createStoreUndoManager` perform `structuredClone` on the workbook, which can be expensive for large documents; consider reusing snapshots or capturing more targeted diffs to reduce cloning overhead during multi-step macro execution and undo operations.
## 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:** Avoid double JSON.parse for SSE events to reduce overhead and edge-case differences.
In `chatWithAgentServerStream`, we `JSON.parse(jsonStr)` to check `type === 'token'`, then call `parseCompleteSseEvent(jsonStr)`, which parses the same string again. This adds per-event overhead and splits token vs. complete handling across two parsing paths. Consider changing `parseCompleteSseEvent` to accept a parsed object (or introducing a helper that branches on a single parsed payload) so each event is parsed once and both code paths share the same parsing logic.
Suggested implementation:
```typescript
/** Parse a pre-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 {
if (event.type !== 'complete' || typeof event.message !== 'string') return null
return {
```
To fully implement the suggestion and avoid double JSON.parse:
1. In `chatWithAgentServerStream`, change the SSE handler so that for each `data:` event you:
- Call `JSON.parse(jsonStr)` once to obtain `const event = ...`.
- Branch on `event.type === 'token'` for token handling.
- Call `parseCompleteSseEvent(event)` for the complete case instead of passing `jsonStr`.
2. Ensure any other call sites of `parseCompleteSseEvent` are updated to pass a parsed event object rather than a string.
3. If you want a shared helper, you can introduce something like:
```ts
function parseSseEventPayload(jsonStr: string): {
type?: string
message?: string
actions?: ServerAgentAction[]
source?: string
reasoning?: string
suggestions?: string[]
meta?: ProviderMeta
} | null {
try {
return JSON.parse(jsonStr)
} catch {
return null
}
}
```
and use it in `chatWithAgentServerStream` before calling `parseCompleteSseEvent`.
</issue_to_address>
### Comment 2
<location path="src/ai/macro/toolStepExecutor.ts" line_range="54" />
<code_context>
+ next.column = next.columns[0]
+ }
+
+ if (tool === 'filter') {
+ const operators = Array.isArray(next.operators) ? next.operators : []
+ const values = Array.isArray(next.values) ? next.values : []
</code_context>
<issue_to_address>
**issue (bug_risk):** Filter-specific param normalization depends on the resolved tool name, which might diverge from the NLP name.
Because `tool` is the resolved name, `if (tool === 'filter')` will fail if the NLP intent `filter` is ever mapped to a different canonical tool (e.g. `filter_sheet`), and the `operators`/`values` normalization won’t run for those tools. Consider branching on the original `step.tool` before resolution, or using a broader condition (e.g. `tool.startsWith('filter')`) so filter-style tools continue to receive normalization as registry names change.
</issue_to_address>
### Comment 3
<location path="scripts/copy-deploy-models.mjs" line_range="113-115" />
<code_context>
+async function downloadFile(url, dest) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Script assumes global fetch in Node; consider guarding or documenting Node version requirements.
This script calls `await fetch(url)` and will throw in Node versions without a global `fetch`. Please either add a runtime check that enforces a minimum Node version with a clear error, or import a small fetch polyfill so the dependency is explicit and the script behaves consistently across environments.
```suggestion
async function downloadFile(url, dest) {
if (typeof globalThis.fetch !== 'function') {
throw new Error(
'scripts/copy-deploy-models.mjs requires a Node.js version with global fetch (Node 18+) or a fetch polyfill'
)
}
console.log(` downloading ${url}`)
const res = await globalThis.fetch(url)
```
</issue_to_address>
### Comment 4
<location path="src/ai/macro/__tests__/toolStepExecutor.test.ts" line_range="36" />
<code_context>
new file mode 100644
index 0000000..a1b767c
--- /dev/null
+++ b/src/ai/macro/__tests__/toolStepExecutor.test.ts
</code_context>
<issue_to_address>
**suggestion (testing):** Extend toolStepExecutor tests to cover filter operator/value normalization and unknown operator behavior
These tests cover sort/format mapping and error propagation, but the new `normalizeStepParams` behavior for filter steps isn’t exercised. Please add: (1) a test where a filter step includes `operators`/`values` and assert that the `condition` and `value` passed to `executeToolAsync` are correctly mapped, and (2) a test with an unknown operator string to confirm we fall back to the raw operator instead of throwing, so the NLP-to-tool execution contract is verified end-to-end.
</issue_to_address>
### Comment 5
<location path="server/package-lock.json" line_range="1648-1656" />
<code_context>
</code_context>
<issue_to_address>
**security (CVE-2026-39244):** adm-zip: Denial of Service via crafted ZIP file leading to excessive memory allocation
adm-zip before 0.5.18 is vulnerable to denial of service via a crafted ZIP file with a manipulated uncompressed size header field. In zipEntry.js line 103, Buffer.alloc(_centralHeader.size) allocates memory based on the declared uncompressed size from the ZIP central directory header without validating it against the actual compressed data size or imposing any upper bound. The size value is read directly from the binary header at entryHeader.js line 266 with no bounds check. An attacker can craft a ~120-byte ZIP file that declares ~4GB uncompressed size, causing a memory allocation amplification ratio of over 33 million to 1. The allocation occurs before CRC validation, so the malicious payload cannot be rejected early. All extraction and read methods are affected: readFile(), readAsText(), extractEntryTo(), extractAllTo(), extractAllToAsync(), test(), and entry.getData(). Any application accepting untrusted ZIP files via adm-zip is vulnerable to immediate process crash.
*Source: trivy*
</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: Avoid double JSON.parse for SSE events to reduce overhead and edge-case differences.
In chatWithAgentServerStream, we JSON.parse(jsonStr) to check type === 'token', then call parseCompleteSseEvent(jsonStr), which parses the same string again. This adds per-event overhead and splits token vs. complete handling across two parsing paths. Consider changing parseCompleteSseEvent to accept a parsed object (or introducing a helper that branches on a single parsed payload) so each event is parsed once and both code paths share the same parsing logic.
Suggested implementation:
/** Parse a pre-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 {
if (event.type !== 'complete' || typeof event.message !== 'string') return null
return {To fully implement the suggestion and avoid double JSON.parse:
-
In
chatWithAgentServerStream, change the SSE handler so that for eachdata:event you:- Call
JSON.parse(jsonStr)once to obtainconst event = .... - Branch on
event.type === 'token'for token handling. - Call
parseCompleteSseEvent(event)for the complete case instead of passingjsonStr.
- Call
-
Ensure any other call sites of
parseCompleteSseEventare updated to pass a parsed event object rather than a string. -
If you want a shared helper, you can introduce something like:
function parseSseEventPayload(jsonStr: string): { type?: string message?: string actions?: ServerAgentAction[] source?: string reasoning?: string suggestions?: string[] meta?: ProviderMeta } | null { try { return JSON.parse(jsonStr) } catch { return null } }
and use it in
chatWithAgentServerStreambefore callingparseCompleteSseEvent.
| next.column = next.columns[0] | ||
| } | ||
|
|
||
| if (tool === 'filter') { |
There was a problem hiding this comment.
issue (bug_risk): Filter-specific param normalization depends on the resolved tool name, which might diverge from the NLP name.
Because tool is the resolved name, if (tool === 'filter') will fail if the NLP intent filter is ever mapped to a different canonical tool (e.g. filter_sheet), and the operators/values normalization won’t run for those tools. Consider branching on the original step.tool before resolution, or using a broader condition (e.g. tool.startsWith('filter')) so filter-style tools continue to receive normalization as registry names change.
Comment on lines
+113
to
+115
| async function downloadFile(url, dest) { | ||
| console.log(` downloading ${url}`) | ||
| const res = await fetch(url) |
There was a problem hiding this comment.
suggestion (bug_risk): Script assumes global fetch in Node; consider guarding or documenting Node version requirements.
This script calls await fetch(url) and will throw in Node versions without a global fetch. Please either add a runtime check that enforces a minimum Node version with a clear error, or import a small fetch polyfill so the dependency is explicit and the script behaves consistently across environments.
Suggested change
| async function downloadFile(url, dest) { | |
| console.log(` downloading ${url}`) | |
| const res = await fetch(url) | |
| async function downloadFile(url, dest) { | |
| if (typeof globalThis.fetch !== 'function') { | |
| throw new Error( | |
| 'scripts/copy-deploy-models.mjs requires a Node.js version with global fetch (Node 18+) or a fetch polyfill' | |
| ) | |
| } | |
| console.log(` downloading ${url}`) | |
| const res = await globalThis.fetch(url) |
| }) | ||
| }) | ||
|
|
||
| describe('createToolStepExecutor', () => { |
There was a problem hiding this comment.
suggestion (testing): Extend toolStepExecutor tests to cover filter operator/value normalization and unknown operator behavior
These tests cover sort/format mapping and error propagation, but the new normalizeStepParams behavior for filter steps isn’t exercised. Please add: (1) a test where a filter step includes operators/values and assert that the condition and value passed to executeToolAsync are correctly mapped, and (2) a test with an unknown operator string to confirm we fall back to the raw operator instead of throwing, so the NLP-to-tool execution contract is verified end-to-end.
Comment on lines
+1648
to
+1656
| "node_modules/adm-zip": { | ||
| "version": "0.5.18", | ||
| "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.5.18.tgz", | ||
| "integrity": "sha512-ufJnssQGbxzLNS1Ho9bCtX4rQKCCvoVuDLHoJyc3F9dOGDB4BkWs2Ci0kv53lqocAEQ/Cbi+I2XCsNYGqVYqng==", | ||
| "license": "MIT", | ||
| "engines": { | ||
| "node": ">=12.0" | ||
| } | ||
| }, |
There was a problem hiding this comment.
security (CVE-2026-39244): adm-zip: Denial of Service via crafted ZIP file leading to excessive memory allocation
adm-zip before 0.5.18 is vulnerable to denial of service via a crafted ZIP file with a manipulated uncompressed size header field. In zipEntry.js line 103, Buffer.alloc(_centralHeader.size) allocates memory based on the declared uncompressed size from the ZIP central directory header without validating it against the actual compressed data size or imposing any upper bound. The size value is read directly from the binary header at entryHeader.js line 266 with no bounds check. An attacker can craft a ~120-byte ZIP file that declares ~4GB uncompressed size, causing a memory allocation amplification ratio of over 33 million to 1. The allocation occurs before CRC validation, so the malicious payload cannot be rejected early. All extraction and read methods are affected: readFile(), readAsText(), extractEntryTo(), extractAllTo(), extractAllToAsync(), test(), and entry.getData(). Any application accepting untrusted ZIP files via adm-zip is vulnerable to immediate process crash.
Source: trivy
There was a problem hiding this comment.
Pull request overview
This PR finishes several “Priority Residuals” items by (1) cutting over the client-side chat pipeline from the legacy BrainDispatcher to a split deterministic + server-LLM gateway, (2) introducing real macro planning/execution with grouped undo behavior, (3) propagating server-reported LLM provider/model metadata end-to-end for dev visibility, and (4) wiring up server-side ONNX “Path B” inference with a model copy/download script plus SSRF-hardened shared BYOK schemas and usage-metering fail-closed behavior.
Changes:
- Replace legacy
brainDispatcherpipeline terminal stage withMacroPlanner→DeterministicDispatcher→LLMGateway, and propagate provider metadata into chat messages. - Implement production macro execution (
execute_macro) with a real step executor and grouped undo snapshots. - Add ONNX Path B server route/session-pool wiring and a deploy-model copy/download script; unify BYOK SSRF schemas across endpoints; harden usage metering on DB failures.
Reviewed changes
Copilot reviewed 44 out of 49 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/api.ts | Adds ProviderMeta and attaches it to ChatMessage for provider/model attribution. |
| src/store/useStore.ts | Adds execute_macro action execution with grouped undo behavior and adjusts history handling. |
| src/services/chatService.ts | Rewires chat pipeline stages (macro planner + deterministic dispatcher + LLM gateway) and maps provider metadata into rendered messages. |
| src/onnx/inputValidator.property.test.ts | Tightens property test assertions to handle small tensor sizes safely. |
| src/components/ChatPanel.tsx | Renders provider/model details in DEV builds for assistant messages when available. |
| src/ai/types.ts | Extends ToolResult to carry provider metadata. |
| src/ai/responseBuilder.ts | Propagates providerMeta from ToolResult into ChatMessage. |
| src/ai/pipeline/stages/macroPlanner.ts | Introduces a macro planning stage that emits a pending execute_macro action for multi-clause commands. |
| src/ai/pipeline/stages/llmGateway.ts | Adds provider metadata from server responses into stage metadata. |
| src/ai/pipeline/stages/index.ts | Exports new pipeline stages (macro planner, deterministic dispatcher) and removes legacy brain dispatcher export. |
| src/ai/pipeline/stages/deterministicDispatcher.ts | Allows resolving analysis target from PipelineContext when not injected (better for production chatService wiring). |
| src/ai/pipeline/stages/brainDispatcher.ts | Removes the legacy transitional BrainDispatcher stage implementation. |
| src/ai/pipeline/index.ts | Updates pipeline exports to reflect the new stage split. |
| src/ai/pipeline/tests/smokeTest.integration.test.ts | Updates integration smoke tests to match the new deterministic + LLM gateway pipeline behavior. |
| src/ai/pipeline/tests/pipeline.integration.test.ts | Updates end-to-end routing tests to reflect new stage order and LLM gateway usage. |
| src/ai/pipeline/tests/macroPlanner.test.ts | Adds unit tests for macro planner stage claim/pass-through behavior. |
| src/ai/macro/toolStepExecutor.ts | Implements a real macro step executor mapping macro steps to executeToolAsync, with param normalization. |
| src/ai/macro/storeUndoManager.ts | Adds a store-backed undo manager that snapshots/restores workbook state for macro transactions. |
| src/ai/macro/macroPlanManager.ts | Requires an injected step executor (no silent stub default) and wires it into macro execution. |
| src/ai/macro/macroExecutor.ts | Makes step executor required for executeMacro and clarifies stub is test-only. |
| src/ai/macro/index.ts | Adds public exports for macro execution/manager helpers and executors. |
| src/ai/macro/tests/toolStepExecutor.test.ts | Adds unit tests for step param normalization and tool mapping to executeToolAsync. |
| src/ai/macro/tests/storeUndoManager.test.ts | Adds unit tests verifying rollback/commit snapshot behavior. |
| src/ai/macro/tests/macroExecutor.test.ts | Updates macro executor tests for required step executor argument. |
| src/ai/brain.ts | Updates legacy orchestrator notes and ensures macro plan manager uses the default test executor for macro unit tests. |
| src/ai/agentClient.ts | Adds parsing for SSE complete events including meta provider info; maps server provider meta onto ChatMessage. |
| src/ai/agentClient.test.ts | Adds unit tests asserting SSE complete event parsing preserves provider metadata. |
| server/src/usage.ts | On DB write failure, bumps in-memory limiter to avoid undercounting during outages (fail-closed-ish behavior). |
| server/src/usage.test.ts | Adds tests for DB failure fallback behavior (check + record paths). |
| server/src/schemas/index.ts | Re-exports shared BYOK schemas/utilities from a central module. |
| server/src/schemas/chat.ts | Switches chat schema to use shared BYOK schema (prevents SSRF validation drift). |
| server/src/schemas/byok.ts | Introduces shared SSRF-hardened BYOK Zod schemas + helper for public HTTPS validation. |
| server/src/schemas/byok.test.ts | Adds tests verifying SSRF BYOK rejection on both chat + ai-function schemas. |
| server/src/schemas/aiFunction.ts | Switches ai-function schema to use shared BYOK schema (prevents SSRF validation drift). |
| server/src/onnx/sessionPool.ts | Updates default model path resolution comment and relies on injected resolver for Path B wiring. |
| server/src/onnx/onnxruntime-node.d.ts | Removes the local type stub now that onnxruntime-node is installed. |
| server/src/onnx/modelPaths.ts | Adds filesystem-based model path resolution utilities for nested vs flat layouts. |
| server/src/onnx/modelPaths.test.ts | Adds unit tests for ONNX model path and size resolution utilities. |
| server/src/index.ts | Mounts /api/onnx with session pool, logs ONNX readiness, warms pool, and disposes on shutdown; adds provider meta tracking for LLM responses including BYOK. |
| server/package.json | Adds onnxruntime-node dependency for server-side inference. |
| server/package-lock.json | Updates lockfile for onnxruntime-node and transitive dependencies. |
| server/models/README.md | Documents server-side ONNX model layout and the copy/download workflow. |
| server/models/.gitkeep | Keeps server/models/ tracked while weights remain gitignored. |
| scripts/copy-deploy-models.mjs | Adds a cross-platform model copy/download script for MiniLM (plus optional GGUF copy). |
| package.json | Adds model:copy-deploy script to run the new deploy model copy/download helper. |
| package-lock.json | Updates root lockfile metadata (and any associated dependency graph updates). |
| models/README.md | Updates model docs to reflect prod Spreadsheet-RL-4B posture and MiniLM Path B copy/deploy workflow. |
| docs/ENV.md | Documents SMARTSHT_MINILM_SRC and ONNX Path B operational notes. |
| .gitignore | Gitignores server ONNX weights while keeping README + .gitkeep tracked. |
Files not reviewed (1)
- server/package-lock.json: Generated file
Suppressed comments (1)
src/ai/agentClient.ts:142
chatWithAgentServerStreamparsesjsonStrand then callsparseCompleteSseEvent(jsonStr), which re-parses the same JSON. After updatingparseCompleteSseEventto accept an already-parsed object, passparsedto avoid the extra JSON.parse on every non-token SSE line.
}
const complete = parseCompleteSseEvent(jsonStr)
if (complete) finalResponse = complete
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
409
to
413
| /** | ||
| * Resolve a model name to a file path. | ||
| * Default prefers nested models/{name}/model.onnx, then flat models/{name}.onnx. | ||
| * Prefer injecting resolveModelPath at mount time (see index.ts + modelPaths.ts). | ||
| */ |
Comment on lines
+122
to
+123
| // Models live under server/models/ when the process cwd is server/. | ||
| const modelsRoot = path.resolve(process.cwd(), 'models') |
Comment on lines
+25
to
48
| 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 | ||
| } | ||
| 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, | ||
| } | ||
| } catch { | ||
| return null | ||
| } | ||
| } |
| @@ -407,7 +407,7 @@ describe('executeMacro', () => { | |||
| describe('injectable step executor', () => { | |||
| it('uses default executor when none provided', async () => { | |||
Comment on lines
+20
to
+22
| export interface MacroPlannerDeps { | ||
| buildExecContext: () => ExecutionContext | ||
| } |
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
Implements the Priority Residuals plan (finish-for-real posture):
meta; BYOK labeled by actual provider/host; DEV chat detailsbrainDispatcherwith DeterministicDispatcher + LLMGatewayonnxruntime-node, mount infer route, bundled MiniLM (copy script); keep user uploadSpreadsheet-RL-4B is already on prod — no re-upload; local copy script only for dev.
Test plan
npx vitest run(client)npm test --prefix server/api/onnx/inferwith MiniLM after copy scriptSummary by Sourcery
Replace the legacy BrainDispatcher with a split deterministic/LLM pipeline, add real macro execution with grouped undo, expose provider metadata in chat, and introduce a server-side ONNX inference path with hardened BYOK and usage metering.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: