Conversation
- Resolve Git merge conflicts (upstream vs stashed)
- Use streamText with runtime.emitEvent('textStream'/'textStreamDone')
- Add wrapAsV2LanguageModel/wrapAsV2EmbeddingModel for AI SDK compat
- Add getSettingAsString, createFetchWithTimeout, verifiedModels cache
- Type runtime as IAgentRuntime (Pick<...>) for backwards/forwards compat
- Use maxOutputTokens for streamText; remove debug console.log
- package.json: @elizaos/core workspace:*, build via build.ts
- Add build.ts (Bun build + tsc declarations), expand .gitignore
- tsconfig/tsup: declarations, skipLibCheck, paths
Made-with: Cursor
WalkthroughTransitioned build to a Bun-based workflow, bumped Changes
Sequence DiagramsequenceDiagram
participant Client
participant Runtime
participant ModelWrapper as Model Wrapper
participant OllamaAPI as Ollama API
participant EventEmitter as Runtime/Event Emitter
Client->>Runtime: generateOllamaText(model, params)
Runtime->>Runtime: ensureModelAvailable(model, providedBaseURL?)
Runtime->>OllamaAPI: GET /api/show or POST /api/pull
OllamaAPI-->>Runtime: status / pull progress
Runtime->>ModelWrapper: wrapAsV2LanguageModel(ollama(model))
activate ModelWrapper
Runtime->>ModelWrapper: streamText(params)
ModelWrapper->>OllamaAPI: streaming request (with fetch+timeout)
loop streaming chunks
OllamaAPI-->>ModelWrapper: text delta
ModelWrapper-->>Runtime: delta chunk
Runtime->>EventEmitter: emit('textStream', delta)
EventEmitter-->>Client: stream update
end
ModelWrapper-->>Runtime: final assembled output
deactivate ModelWrapper
Runtime->>EventEmitter: emit('textStreamDone', true)
EventEmitter-->>Client: streaming complete
Runtime-->>Client: return final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Made-with: Cursor
odilitime
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/index.ts
Outdated
|
|
||
| const out = [] | ||
| for await (const delta of result.textStream) { | ||
| process.stdout.write(delta); |
There was a problem hiding this comment.
Debug output left in production code
process.stdout.write(delta) was added alongside the new streaming loop, printing every streamed token directly to stdout. The PR description says "remove debug console.log" but this equivalent debug output was introduced. In a server-side plugin this pollutes stdout for every text generation call and may interfere with process output consumers.
| process.stdout.write(delta); | |
| out.push(delta) |
build.ts
Outdated
| if (true) { // Always generate .d.ts | ||
| console.log("📝 Generating TypeScript declarations..."); | ||
| try { | ||
| await $`tsc --project tsconfig.build.json`; | ||
| console.log(`✅ Declarations generated in ${((Date.now() - dtsStart) / 1000).toFixed(2)}s`); | ||
| } catch (error) { | ||
| console.warn(`⚠️ TypeScript declaration generation had errors (${((Date.now() - dtsStart) / 1000).toFixed(2)}s)`); | ||
| console.warn(" Build will continue - fix type errors when possible"); | ||
| } | ||
| } else { | ||
| console.log("🔍 Type checking..."); | ||
| try { | ||
| await $`tsc --noEmit --incremental --project tsconfig.build.json`; | ||
| console.log(`✅ Type check passed in ${((Date.now() - dtsStart) / 1000).toFixed(2)}s`); | ||
| } catch (error) { | ||
| console.warn(`⚠️ Type checking had errors (${((Date.now() - dtsStart) / 1000).toFixed(2)}s)`); | ||
| console.warn(" Build will continue - fix type errors when possible"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Dead code: hardcoded if (true) makes else branch unreachable
The if (true) guard was likely left as a placeholder during development. The entire else block (type-check-only path) can never execute, which means it is dead code that will mislead anyone reading the file and is never tested.
| if (true) { // Always generate .d.ts | |
| console.log("📝 Generating TypeScript declarations..."); | |
| try { | |
| await $`tsc --project tsconfig.build.json`; | |
| console.log(`✅ Declarations generated in ${((Date.now() - dtsStart) / 1000).toFixed(2)}s`); | |
| } catch (error) { | |
| console.warn(`⚠️ TypeScript declaration generation had errors (${((Date.now() - dtsStart) / 1000).toFixed(2)}s)`); | |
| console.warn(" Build will continue - fix type errors when possible"); | |
| } | |
| } else { | |
| console.log("🔍 Type checking..."); | |
| try { | |
| await $`tsc --noEmit --incremental --project tsconfig.build.json`; | |
| console.log(`✅ Type check passed in ${((Date.now() - dtsStart) / 1000).toFixed(2)}s`); | |
| } catch (error) { | |
| console.warn(`⚠️ Type checking had errors (${((Date.now() - dtsStart) / 1000).toFixed(2)}s)`); | |
| console.warn(" Build will continue - fix type errors when possible"); | |
| } | |
| } | |
| // Always generate .d.ts | |
| console.log("📝 Generating TypeScript declarations..."); | |
| try { | |
| await $`tsc --project tsconfig.build.json`; | |
| console.log(`✅ Declarations generated in ${((Date.now() - dtsStart) / 1000).toFixed(2)}s`); | |
| } catch (error) { | |
| console.warn(`⚠️ TypeScript declaration generation had errors (${((Date.now() - dtsStart) / 1000).toFixed(2)}s)`); | |
| console.warn(" Build will continue - fix type errors when possible"); | |
| } |
package.json
Outdated
| "build": "tsup", | ||
| "dev": "tsup --watch", | ||
| "build": "bun run build.ts", | ||
| "dev": "bun run build.ts --watch", |
There was a problem hiding this comment.
dev --watch flag is silently ignored
The new dev script passes --watch to build.ts as a CLI argument, but build.ts never reads process.argv or Bun.argv, so the flag has no effect. Running bun run dev will perform a single build and exit — it will not watch for changes as the script name implies.
Either implement watch mode in build.ts (e.g., via Bun.build({ watch: true, … })) or remove the --watch flag until the feature is ready.
| return async (input: RequestInfo | URL, init?: RequestInit) => { | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), timeoutMs); | ||
|
|
||
| try { | ||
| const response = await fetchFn(input, { | ||
| ...init, | ||
| signal: controller.signal, | ||
| }); | ||
| return response; | ||
| } finally { | ||
| clearTimeout(timeoutId); | ||
| } |
There was a problem hiding this comment.
Caller-provided AbortSignal is silently discarded
createFetchWithTimeout unconditionally overwrites init.signal with the timeout controller's own signal:
signal: controller.signal, // overwrites init?.signalIf the caller (e.g., the ollama-ai-provider internals) passes its own abort signal in init, that signal is ignored. The request will no longer be cancellable by the caller, which can lead to zombie requests that run to completion even after the upstream operation has been aborted.
A safer approach is to combine both signals:
const signals = [controller.signal, init?.signal].filter(Boolean) as AbortSignal[];
const signal = signals.length > 1 ? AbortSignal.any(signals) : signals[0];
const response = await fetchFn(input, { ...init, signal });| "@elizaos/core": "1.7.2", | ||
| "ai": "^4.3.9", | ||
| "js-tiktoken": "^1.0.18", | ||
| "ollama-ai-provider": "^1.2.0", |
There was a problem hiding this comment.
tsup listed as a runtime dependency
tsup is a build tool and should be in devDependencies, not dependencies. Keeping it in dependencies causes it to be installed in production environments and inflates the package's install footprint for consumers.
| "ollama-ai-provider": "^1.2.0", | |
| "tsup": "8.4.0" |
Move the line to devDependencies.
There was a problem hiding this comment.
Pull request overview
This PR updates the Ollama plugin to support AI SDK streaming and improve compatibility with AI SDK v2 model interfaces, while also changing the build/declaration generation pipeline (Bun build + tsc).
Changes:
- Switch text generation to
streamText, emittingtextStream/textStreamDoneruntime events, and add v2 wrapper proxies for language/embedding models. - Add runtime setting helpers, a fetch timeout wrapper, and a per-process cache for verified models.
- Replace tsup-based builds with a Bun-based build script, adjust TS configs for declaration output, and update package/dependency config.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/index.ts |
Adds streaming, v2 model wrappers, fetch timeout wrapper, settings helper, and verified-model caching. |
build.ts |
Introduces Bun build + tsc declarations generation. |
package.json |
Switches build scripts to Bun build script and changes @elizaos/core dependency to workspace:*. |
tsup.config.ts |
Enables dts but contains now-misleading comments. |
tsconfig.json |
Formatting/paths adjustments. |
tsconfig.build.json |
Declaration-focused build config tweaks (include set, skipLibCheck, paths override). |
.gitignore |
Expanded ignores for build artifacts, IDE files, caches, etc. |
Comments suppressed due to low confidence (1)
src/index.ts:167
- ensureModelAvailable uses the global fetch(), ignoring runtime.fetch and the new timeout wrapper. This can bypass custom fetch implementations (proxy, auth, instrumentation) and can hang indefinitely. Use createFetchWithTimeout(runtime.fetch) (or at least runtime.fetch ?? fetch) for the /api/show and /api/pull requests.
async function ensureModelAvailable(
runtime: Pick<IAgentRuntime, "getSetting"> & { fetch?: typeof fetch },
model: string,
providedBaseURL?: string
) {
const baseURL = providedBaseURL || getBaseURL(runtime);
// Remove /api suffix for direct API calls
const apiBase = baseURL.endsWith("/api") ? baseURL.slice(0, -4) : baseURL;
const cacheKey = `${apiBase}:${model}`;
if (verifiedModels.has(cacheKey)) return;
try {
const showRes = await fetch(`${apiBase}/api/show`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ model }),
});
if (showRes.ok) {
verifiedModels.add(cacheKey);
return;
}
logger.info(`[Ollama] Model ${model} not found locally. Downloading...`);
const pullRes = await fetch(`${apiBase}/api/pull`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ model, stream: false }),
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| } catch (error: unknown) { | ||
| logger.error({ error }, 'Error in generateOllamaText'); | ||
| return 'Error generating text. Please try again later.'; | ||
| logger.error({ err: error instanceof Error ? error : new Error(String(error)) }, "Error in generateOllamaText"); | ||
| return "Error generating text. Please try again later."; |
There was a problem hiding this comment.
The structured logging key was changed to err here, but the rest of this file consistently logs errors under { error: ... }. Using inconsistent keys makes log querying/alerting harder; consider keeping the same key across the plugin (e.g., { error: normalizedError }).
| const baseURL = getBaseURL(runtime); | ||
| const ollama = createOllama({ | ||
| fetch: runtime.fetch, | ||
| baseURL, | ||
| }); |
There was a problem hiding this comment.
TEXT_EMBEDDING still constructs the provider with fetch: runtime.fetch instead of the new timeout wrapper. This makes embedding calls behave differently than TEXT_SMALL/LARGE (and can still hang on long requests). Consider using createFetchWithTimeout(runtime.fetch) consistently for all Ollama provider instances.
src/index.ts
Outdated
| } from "@elizaos/core"; | ||
| import { type GenerateTextParams, ModelType, logger } from "@elizaos/core"; | ||
| import { generateObject, streamText, embed } from "ai"; | ||
| import type { EmbeddingModel, LanguageModel } from "ai"; | ||
| import { createOllama } from "ollama-ai-provider"; |
There was a problem hiding this comment.
The new imports use double quotes while the rest of the repo (and this file below) predominantly uses single quotes. This will keep causing noisy diffs unless formatting is applied consistently (e.g., run prettier or align the quote style).
| } from "@elizaos/core"; | |
| import { type GenerateTextParams, ModelType, logger } from "@elizaos/core"; | |
| import { generateObject, streamText, embed } from "ai"; | |
| import type { EmbeddingModel, LanguageModel } from "ai"; | |
| import { createOllama } from "ollama-ai-provider"; | |
| } from '@elizaos/core'; | |
| import { type GenerateTextParams, ModelType, logger } from '@elizaos/core'; | |
| import { generateObject, streamText, embed } from 'ai'; | |
| import type { EmbeddingModel, LanguageModel } from 'ai'; | |
| import { createOllama } from 'ollama-ai-provider'; |
package.json
Outdated
| "build": "tsup", | ||
| "dev": "tsup --watch", | ||
| "build": "bun run build.ts", | ||
| "dev": "bun run build.ts --watch", |
There was a problem hiding this comment.
The dev script passes --watch to build.ts, but build.ts never reads argv or enables Bun.build watch mode / tsc watch. As-is, bun run build.ts --watch will do a one-off build, so dev is misleading/broken. Either implement watch handling in build.ts or change the script to a command that actually watches.
| "dev": "bun run build.ts --watch", | |
| "dev": "bun --watch run build.ts", |
| // Use the clean script from package.json | ||
| if (pkg.scripts?.clean) { | ||
| console.log("🧹 Cleaning..."); | ||
| await $`bun run clean`.quiet(); |
There was a problem hiding this comment.
build.ts always runs the package's clean script before building. In this repo the clean script removes node_modules, so running build/dev will delete installed dependencies and can break or severely slow builds. Consider limiting clean to build artifacts (e.g., dist/tsbuildinfo) or make cleaning opt-in via a flag.
| // Use the clean script from package.json | |
| if (pkg.scripts?.clean) { | |
| console.log("🧹 Cleaning..."); | |
| await $`bun run clean`.quiet(); | |
| // Use the clean script from package.json (opt-in via --clean) | |
| const shouldClean = process.argv.includes("--clean"); | |
| if (pkg.scripts?.clean && shouldClean) { | |
| console.log("🧹 Cleaning..."); | |
| await $`bun run clean`.quiet(); | |
| } else if (pkg.scripts?.clean && !shouldClean) { | |
| console.log("ℹ️ Skipping clean step (run with --clean to execute `bun run clean`)"); |
tsup.config.ts
Outdated
| clean: true, | ||
| format: ['esm'], // Ensure you're targeting CommonJS | ||
| dts: false, // Skip DTS generation to avoid external import issues // Ensure you're targeting CommonJS | ||
| dts: true, // Skip DTS generation to avoid external import issues // Ensure you're targeting CommonJS |
There was a problem hiding this comment.
tsup.config has contradictory comments: format: ['esm'] is annotated as targeting CommonJS, and dts: true is annotated as skipping DTS generation. Update/remove these comments to avoid misleading future maintainers (or adjust the options to match the intent).
src/index.ts
Outdated
| const fetchFn = baseFetch ?? ((input: RequestInfo | URL, init?: RequestInit) => fetch(input, init)); | ||
| return async (input: RequestInfo | URL, init?: RequestInit) => { | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), timeoutMs); | ||
|
|
||
| try { | ||
| const response = await fetchFn(input, { | ||
| ...init, | ||
| signal: controller.signal, |
There was a problem hiding this comment.
createFetchWithTimeout always overwrites any existing init.signal with its own AbortController signal. This breaks caller-provided abort semantics (and can cause requests to ignore upstream cancellation). Merge the existing signal with the timeout signal (e.g., AbortSignal.any when available, or manually forward abort) instead of unconditionally replacing it.
| const fetchFn = baseFetch ?? ((input: RequestInfo | URL, init?: RequestInit) => fetch(input, init)); | |
| return async (input: RequestInfo | URL, init?: RequestInit) => { | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), timeoutMs); | |
| try { | |
| const response = await fetchFn(input, { | |
| ...init, | |
| signal: controller.signal, | |
| const fetchFn = | |
| baseFetch ?? ((input: RequestInfo | URL, init?: RequestInit) => fetch(input, init)); | |
| return async (input: RequestInfo | URL, init?: RequestInit) => { | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), timeoutMs); | |
| const existingSignal = init?.signal; | |
| let signalToUse: AbortSignal; | |
| if (existingSignal) { | |
| // If AbortSignal.any is available, create a combined signal that aborts when | |
| // either the caller's signal or the timeout controller aborts. | |
| if (typeof AbortSignal !== "undefined" && "any" in AbortSignal) { | |
| // AbortSignal.any may not be declared in older lib typings, so use `any` to access it. | |
| signalToUse = (AbortSignal as any).any([existingSignal, controller.signal]); | |
| } else { | |
| // Fallback: forward abort from the existing signal to our controller. | |
| if (existingSignal.aborted) { | |
| controller.abort((existingSignal as any).reason); | |
| } else { | |
| existingSignal.addEventListener( | |
| "abort", | |
| () => controller.abort((existingSignal as any).reason), | |
| { once: true } | |
| ); | |
| } | |
| signalToUse = controller.signal; | |
| } | |
| } else { | |
| signalToUse = controller.signal; | |
| } | |
| try { | |
| const response = await fetchFn(input, { | |
| ...init, | |
| signal: signalToUse, |
| const out = [] | ||
| for await (const delta of result.textStream) { | ||
| process.stdout.write(delta); | ||
| runtime.emitEvent('textStream', delta) | ||
| out.push(delta) |
There was a problem hiding this comment.
generateOllamaText writes streamed tokens directly to process.stdout. As a library/plugin, this causes unexpected side effects (noise in host apps, breaks structured logging, and can leak content in server environments). Prefer emitting events only (or guard stdout writes behind a debug setting).
build.ts
Outdated
| if (true) { // Always generate .d.ts | ||
| console.log("📝 Generating TypeScript declarations..."); | ||
| try { | ||
| await $`tsc --project tsconfig.build.json`; | ||
| console.log(`✅ Declarations generated in ${((Date.now() - dtsStart) / 1000).toFixed(2)}s`); | ||
| } catch (error) { | ||
| console.warn(`⚠️ TypeScript declaration generation had errors (${((Date.now() - dtsStart) / 1000).toFixed(2)}s)`); | ||
| console.warn(" Build will continue - fix type errors when possible"); | ||
| } |
There was a problem hiding this comment.
The declarations build catches tsc failures and continues. That can produce a JS build with missing/stale/broken .d.ts, which is especially risky for a published package. Consider failing the build on declaration errors (or gating the "continue on error" behavior behind an explicit env var/flag).
| const result = streamText({ | ||
| model: wrapAsV2LanguageModel(ollama(model)), | ||
| prompt: params.prompt, | ||
| system: params.system, | ||
| temperature: params.temperature, | ||
| maxTokens: params.maxTokens, | ||
| maxOutputTokens: params.maxTokens, | ||
| frequencyPenalty: params.frequencyPenalty, | ||
| presencePenalty: params.presencePenalty, | ||
| stopSequences: params.stopSequences, | ||
| }); |
There was a problem hiding this comment.
New streaming behavior (streamText + emitEvent textStream/textStreamDone) and the timeout wrapper are not covered by the existing bun tests. Consider adding unit tests with a mocked runtime/model to verify events are emitted in order and that timeout/abort behavior works as expected.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
342-349:⚠️ Potential issue | 🟡 MinorFallback embedding dimension (1536) may not match actual model dimension.
The fallback
Array(1536).fill(0)uses OpenAI's embedding dimension, but the default modelnomic-embed-text:latestproduces 768-dimensional embeddings. This mismatch could cause issues if fallback embeddings are stored alongside real embeddings.Consider dynamically determining the dimension or using a dimension that matches the configured model:
♻️ Suggested improvement
} catch (embeddingError) { logger.error({ error: embeddingError }, 'Error generating embedding'); - return Array(1536).fill(0); + // nomic-embed-text produces 768 dimensions; adjust if using different model + return Array(768).fill(0); } } catch (error) { logger.error({ error }, 'Error in TEXT_EMBEDDING model'); // Return a fallback vector rather than crashing - return Array(1536).fill(0); + return Array(768).fill(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 342 - 349, The fallback returns Array(1536).fill(0) which may not match the configured embedding model dimension (e.g., nomic-embed-text:latest → 768); update the error handlers around embeddingError and the outer TEXT_EMBEDDING catch to compute the fallback length dynamically (e.g., derive dimension from the configured model name/constant or from a cached successful embedding length) instead of hardcoding 1536 so the fallback produced by the return Array(...).fill(0) matches the actual model dimension.
🧹 Nitpick comments (2)
tsup.config.ts (1)
9-10: This tsup configuration is unused—consider removing it or updating comments.Based on the PR changes,
package.jsonnow runsbun run build.tsfor the build script, andbuild.tsusesBun.build()withtscfor declarations. Thistsup.config.tsfile is never invoked.Additionally, the comments are contradictory:
- Line 9: States "Ensure you're targeting CommonJS" but
formatis['esm']- Line 10: States "Skip DTS generation" but
dts: trueenables itIf tsup is no longer part of the build pipeline, consider removing this file and the
tsupdependency frompackage.json. If it's kept for fallback purposes, update the comments to reflect accurate behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsup.config.ts` around lines 9 - 10, The tsup.config.ts is stale and not used by the new build flow; either remove the file and the tsup dependency from package.json, or keep it as a documented fallback and correct its settings/comments—update the comment for format to reflect that format: ['esm'] targets ESM (not CommonJS) and change the dts comment to state that dts: true enables declaration generation; ensure any retained tsup config is actually referenced by build scripts (or delete it and remove tsup from dependencies).src/index.ts (1)
79-98: User-providedsignalis overwritten, potentially breaking caller's abort handling.If
initalready contains asignalproperty (e.g., caller wants to abort on their own condition), it gets silently overwritten by the timeout controller's signal on line 91.Consider using
AbortSignal.any()(available in Node.js 20+) to combine signals, or at minimum document that caller signals are not supported:♻️ Suggested fix using AbortSignal.any
function createFetchWithTimeout( baseFetch: FetchLike | undefined, timeoutMs: number = DEFAULT_TIMEOUT_MS ): FetchLike { const fetchFn = baseFetch ?? ((input: RequestInfo | URL, init?: RequestInit) => fetch(input, init)); return async (input: RequestInfo | URL, init?: RequestInit) => { const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), timeoutMs); + // Combine timeout signal with any user-provided signal + const signals = [controller.signal]; + if (init?.signal) signals.push(init.signal); + const combinedSignal = signals.length > 1 + ? AbortSignal.any(signals) + : controller.signal; try { const response = await fetchFn(input, { ...init, - signal: controller.signal, + signal: combinedSignal, }); return response; } finally { clearTimeout(timeoutId); } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 79 - 98, The createFetchWithTimeout function currently overwrites a caller-provided init.signal with the timeout AbortController, breaking external abort handling; update createFetchWithTimeout to merge signals by using AbortSignal.any([userSignal, timeoutController.signal]) when AbortSignal.any is available (Node 20+), and fall back to wiring the timeout controller while respecting the existing signal (e.g., listen to userSignal and abort timeout controller or vice versa) so the caller's signal is not lost; ensure references to fetchFn, the created controller, timeoutId, and the init spread remain correct and that the final passed signal is the combined/compatible signal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.ts`:
- Around line 36-54: The if (true) in build.ts makes the type-check-only else
branch unreachable; either remove the dead else branch (the console.log("🔍 Type
checking...") block and its try/catch) or make it configurable by replacing the
constant with a flag (e.g., use an env var or CLI option) and branch on that
flag; update references around dtsStart and the tsc invocations (tsc --project
tsconfig.build.json vs tsc --noEmit --incremental --project tsconfig.build.json)
so only the desired path is executed and remove the unused code to satisfy the
noConstantCondition rule.
In `@package.json`:
- Around line 33-34: The dev script passes --watch to build.ts but build.ts
doesn't read CLI flags, so implement watch handling: update build.ts to parse
Bun.argv (or a flag parser) and add a watch mode that runs the existing build
logic repeatedly or hooks into a file watcher; specifically detect the presence
of "--watch" (or "--watch=true") in Bun.argv inside build.ts and, when present,
start a file watcher loop that re-runs the build routine (the same function
currently performing the single build) on file changes, or alternatively remove
the flag from package.json and use a separate watcher tool—ensure changes
reference build.ts and Bun.argv (or the chosen flag parser) and reuse the
existing build function to avoid duplication.
In `@src/index.ts`:
- Around line 199-223: Remove the library-side console side-effect by deleting
the process.stdout.write(delta) call in the streaming loop and rely on
runtime.emit to deliver chunks; replace all calls to
runtime.emitEvent('textStream', delta) and runtime.emitEvent('textStreamDone',
true) with runtime.emit('textStream', delta) and runtime.emit('textStreamDone',
true') respectively (fixing the method name), and update the function parameter
type from Pick<IAgentRuntime, "emitEvent"> to Pick<IAgentRuntime, "emit"> so the
signature matches the `@elizaos/core` 1.7.2 API.
In `@tsconfig.build.json`:
- Line 11: tsconfig.build.json currently sets "paths": {} which overrides and
removes the parent tsconfig path mapping for `@elizaos/core` causing tsc
declaration generation to fail; fix by either removing the empty "paths" entry
so the build config inherits the parent's paths, or add a proper path mapping
for "@elizaos/core" that points to the local package source, or ensure
`@elizaos/core` is installed into node_modules before running tsc. Also change the
build.ts error handler (the tsc invocation catch around lines 41-44) to surface
failures (exit non‑zero or rethrow) instead of swallowing the error so missing
.d.ts generation is not silently ignored.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 342-349: The fallback returns Array(1536).fill(0) which may not
match the configured embedding model dimension (e.g., nomic-embed-text:latest →
768); update the error handlers around embeddingError and the outer
TEXT_EMBEDDING catch to compute the fallback length dynamically (e.g., derive
dimension from the configured model name/constant or from a cached successful
embedding length) instead of hardcoding 1536 so the fallback produced by the
return Array(...).fill(0) matches the actual model dimension.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 79-98: The createFetchWithTimeout function currently overwrites a
caller-provided init.signal with the timeout AbortController, breaking external
abort handling; update createFetchWithTimeout to merge signals by using
AbortSignal.any([userSignal, timeoutController.signal]) when AbortSignal.any is
available (Node 20+), and fall back to wiring the timeout controller while
respecting the existing signal (e.g., listen to userSignal and abort timeout
controller or vice versa) so the caller's signal is not lost; ensure references
to fetchFn, the created controller, timeoutId, and the init spread remain
correct and that the final passed signal is the combined/compatible signal.
In `@tsup.config.ts`:
- Around line 9-10: The tsup.config.ts is stale and not used by the new build
flow; either remove the file and the tsup dependency from package.json, or keep
it as a documented fallback and correct its settings/comments—update the comment
for format to reflect that format: ['esm'] targets ESM (not CommonJS) and change
the dts comment to state that dts: true enables declaration generation; ensure
any retained tsup config is actually referenced by build scripts (or delete it
and remove tsup from dependencies).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8681e56e-2d85-4a03-808e-58d183fcaa2f
📒 Files selected for processing (7)
.gitignorebuild.tspackage.jsonsrc/index.tstsconfig.build.jsontsconfig.jsontsup.config.ts
package.json
Outdated
| "build": "bun run build.ts", | ||
| "dev": "bun run build.ts --watch", |
There was a problem hiding this comment.
The dev script's --watch flag is not handled by build.ts.
The dev script passes --watch to build.ts, but build.ts doesn't parse command-line arguments or implement watch mode. The script will run once and exit, not providing the expected continuous rebuild behavior.
Consider either:
- Implementing watch mode in
build.tsusingBun.argvor a flag parser - Using a different approach for dev mode (e.g., keeping tsup for watch mode)
♻️ Example watch mode implementation
+const watchMode = Bun.argv.includes("--watch");
+
async function build() {
// ... existing build logic ...
}
-build().catch((err) => {
- console.error("Build failed:", err);
- process.exit(1);
-});
+if (watchMode) {
+ const { watch } = await import("fs");
+ console.log("👀 Watching for changes...");
+ build(); // Initial build
+ watch("src", { recursive: true }, async (event, filename) => {
+ console.log(`\n📝 ${filename} changed, rebuilding...`);
+ await build();
+ });
+} else {
+ build().catch((err) => {
+ console.error("Build failed:", err);
+ process.exit(1);
+ });
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 33 - 34, The dev script passes --watch to build.ts
but build.ts doesn't read CLI flags, so implement watch handling: update
build.ts to parse Bun.argv (or a flag parser) and add a watch mode that runs the
existing build logic repeatedly or hooks into a file watcher; specifically
detect the presence of "--watch" (or "--watch=true") in Bun.argv inside build.ts
and, when present, start a file watcher loop that re-runs the build routine (the
same function currently performing the single build) on file changes, or
alternatively remove the flag from package.json and use a separate watcher
tool—ensure changes reference build.ts and Bun.argv (or the chosen flag parser)
and reuse the existing build function to avoid duplication.
| "emitDeclarationOnly": true | ||
| "emitDeclarationOnly": true, | ||
| "skipLibCheck": true, | ||
| "paths": {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if `@elizaos/core` is available in node_modules for tsc resolution
echo "=== Checking if `@elizaos/core` exists in node_modules ==="
if [ -d "node_modules/@elizaos/core" ]; then
echo "✅ `@elizaos/core` found in node_modules"
ls -la node_modules/@elizaos/core/
else
echo "❌ `@elizaos/core` NOT found in node_modules - tsc may fail to resolve imports"
fi
echo ""
echo "=== Attempting declaration generation to verify ==="
# Run tsc to see if it succeeds or fails
tsc --project tsconfig.build.json --noEmit 2>&1 | head -30Repository: elizaos-plugins/plugin-ollama
Length of output: 2927
🏁 Script executed:
# Check tsconfig.build.json full content
echo "=== tsconfig.build.json content ==="
cat tsconfig.build.json
echo ""
echo "=== build.ts lines around 41-44 ==="
wc -l build.ts
head -50 build.ts | tail -20Repository: elizaos-plugins/plugin-ollama
Length of output: 1466
🏁 Script executed:
# Check if declaration generation is even executed
echo "=== Search for declaration generation in build.ts ==="
grep -n "declaration\|\.d\.ts\|emitDeclarationOnly" build.tsRepository: elizaos-plugins/plugin-ollama
Length of output: 357
Fix paths: {} override or ensure @elizaos/core is resolved during declaration generation.
Setting "paths": {} in tsconfig.build.json shadows the @elizaos/core path aliases defined in the parent tsconfig.json. During declaration generation (tsc --project tsconfig.build.json), TypeScript attempts to resolve @elizaos/core imports from src/index.ts but fails with TS2307 errors when the package is not in node_modules—common in monorepo setups using workspace dependencies.
The build.ts error handler (lines 41-44) catches these tsc failures and continues the build with a warning, silently preventing .d.ts files from being generated. This breaks the library for TypeScript consumers.
Options to fix:
- Remove
"paths": {}and inherit path mappings from the parent config, OR - Use path mappings that resolve
@elizaos/corecorrectly in your build environment, OR - Install
@elizaos/coreinnode_modulesbefore running the build
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tsconfig.build.json` at line 11, tsconfig.build.json currently sets "paths":
{} which overrides and removes the parent tsconfig path mapping for
`@elizaos/core` causing tsc declaration generation to fail; fix by either removing
the empty "paths" entry so the build config inherits the parent's paths, or add
a proper path mapping for "@elizaos/core" that points to the local package
source, or ensure `@elizaos/core` is installed into node_modules before running
tsc. Also change the build.ts error handler (the tsc invocation catch around
lines 41-44) to surface failures (exit non‑zero or rethrow) instead of
swallowing the error so missing .d.ts generation is not silently ignored.
- Add scripts/rewrite-core-version.ts to set @elizaos/core to publishCoreVersion or restore workspace:* (version from package.json publishCoreVersion or CORE_VERSION) - Add publishCoreVersion 1.7.2, prepare-publish, restore-workspace, prepublishOnly - Add git hooks: pre-commit (commit verified version), post-checkout (restore workspace in monorepo) - Add scripts/install-git-hooks.ts and install-git-hooks script - Document Publishing (standalone repo) and Git hooks in README Made-with: Cursor
Iteration 1 prr-fix:prrc_kwdoojilkm6tt2yl prr-fix:prrc_kwdoojilkm6tt235 prr-fix:prrc_kwdoojilkm6tt51b prr-fix:prrc_kwdoojilkm6tt521 prr-fix:prrc_kwdoojilkm6tt53u prr-fix:prrc_kwdoojilkm6tt6ua prr-fix:prrc_kwdoojilkm6tudjb prr-fix:prrc_kwdoojilkm6turee
Iteration 1 prr-fix:prrc_kwdoojilkm6tt52b
Iteration 1 prr-fix:prrc_kwdoojilkm6tt6up
Iteration 1 prr-fix:prrc_kwdoojilkm6tudjb
Iteration 2 prr-fix:prrc_kwdoojilkm6tt53t
Iteration 1 prr-fix:prrc_kwdoojilkm6tt50z
Explains reasoning for dismissed issues inline in code
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/index.test.ts (1)
61-65:⚠️ Potential issue | 🔴 CriticalOrphaned code outside test block - syntax error.
Lines 62-64 contain assertions (
expect(ollamaPlugin.providers)...) that appear outside anyit()block. This looks like remnants from a previous test that wasn't properly removed or merged.expect(mockRuntime.emit).toHaveBeenCalledWith('textStreamDone', true); }); - expect(ollamaPlugin.providers).toBeDefined(); - expect(Array.isArray(ollamaPlugin.providers)).toBe(true); - }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.test.ts` around lines 61 - 65, The assertions referencing ollamaPlugin.providers are orphaned outside any test block and cause a syntax error; either remove these lines or move them into an existing or new it() test inside the surrounding describe() so they run as part of a test. Locate the stray expects for ollamaPlugin.providers in src/index.test.ts and ensure they are enclosed within an it(...) callback (or delete them if redundant), keeping the symbols ollamaPlugin, providers, expect, it(), and describe() in mind when placing the assertions.src/index.ts (1)
164-199:⚠️ Potential issue | 🟡 MinorInconsistent fetch usage: Uses global
fetchinstead ofruntime.fetch.The function signature accepts
runtime.fetchbut lines 175 and 185 use the globalfetch. This bypasses any custom fetch configuration (like timeout wrapping) used elsewhere in this plugin and could cause inconsistent timeout behavior.🛠️ Proposed fix
async function ensureModelAvailable( runtime: Pick<IAgentRuntime, "getSetting"> & { fetch?: typeof fetch }, model: string, providedBaseURL?: string ) { const baseURL = providedBaseURL || getBaseURL(runtime); // Remove /api suffix for direct API calls const apiBase = baseURL.endsWith("/api") ? baseURL.slice(0, -4) : baseURL; const cacheKey = `${apiBase}:${model}`; if (verifiedModels.has(cacheKey)) return; + const fetchFn = runtime.fetch ?? fetch; try { - const showRes = await fetch(`${apiBase}/api/show`, { + const showRes = await fetchFn(`${apiBase}/api/show`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ model }), }); if (showRes.ok) { verifiedModels.add(cacheKey); return; } logger.info(`[Ollama] Model ${model} not found locally. Downloading...`); - const pullRes = await fetch(`${apiBase}/api/pull`, { + const pullRes = await fetchFn(`${apiBase}/api/pull`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ model, stream: false }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 164 - 199, The ensureModelAvailable function is calling the global fetch instead of using the injected runtime.fetch (so custom timeouts/config are bypassed); update both fetch calls inside ensureModelAvailable to use runtime.fetch when available (e.g., runtime.fetch || fetch) so any wrapped/fallback fetch on the runtime is honored, keeping existing request shape and error handling; references: ensureModelAvailable, runtime.fetch, verifiedModels, getBaseURL.
🧹 Nitpick comments (4)
package.json (1)
30-30:tsupis a dead dependency.The
buildanddevscripts now usebun run build.ts, which invokesBun.buildandtscdirectly. Thetsuppackage is no longer used anywhere in the project.Consider removing it from
dependenciesto reduce package size and avoid confusion.♻️ Remove unused dependency
"js-tiktoken": "^1.0.18", - "ollama-ai-provider": "^1.2.0", - "tsup": "8.4.0" + "ollama-ai-provider": "^1.2.0" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 30, The package.json still lists the unused dependency "tsup" even though the build and dev scripts now call bun run build.ts (which uses Bun.build and tsc directly); remove the "tsup" entry from package.json dependencies so it's no longer installed and update any lockfile (run bun install / npm prune or regenerate lock) to reflect the removal; ensure there are no remaining references to "tsup" in scripts or build files (search for "tsup" and remove/replace if found) to avoid confusion.build.ts (1)
36-39: Type errors now fail the build entirely.The
tscinvocation lacks error handling, so any TypeScript declaration errors will cause the build to fail with a non-zero exit code. This is stricter than a previous iteration which warned and continued.If this strictness is intentional (recommended), consider adding a comment to clarify. If you want to allow builds to succeed despite type errors:
♻️ Optional: Add error handling to continue on type errors
const dtsStart = Date.now(); console.log("📝 Generating TypeScript declarations..."); - await $`tsc --project tsconfig.build.json`; - console.log(`✅ Declarations generated in ${((Date.now() - dtsStart) / 1000).toFixed(2)}s`); + try { + await $`tsc --project tsconfig.build.json`; + console.log(`✅ Declarations generated in ${((Date.now() - dtsStart) / 1000).toFixed(2)}s`); + } catch (error) { + console.warn(`⚠️ Declaration generation had errors (${((Date.now() - dtsStart) / 1000).toFixed(2)}s)`); + console.warn(" Build will continue - fix type errors when possible"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.ts` around lines 36 - 39, The current await $`tsc --project tsconfig.build.json` call will throw on TypeScript errors and abort the build; wrap the invocation in a try/catch (around the await $`tsc...`) so you can log a warning including the error and continue (using process.exitCode if desired), or if strict failure is intended, add an inline comment near dtsStart/console.log and the $`tsc --project tsconfig.build.json` invocation explaining that type errors should fail the build; ensure the catch logs the error and the duration calculation that uses dtsStart still runs.tsup.config.ts (1)
1-20: This config file is dead code.Per the CI workflow (
.github/workflows/npm-deploy.yml:89) andpackage.jsonscripts,bun run buildresolves tobun run build.ts, which usesBun.buildfor bundling andtscfor declarations. This tsup config is never invoked.Additionally,
tsupremains as a runtime dependency inpackage.json(line 30) despite being unused.Consider removing this file and the
tsupdependency to reduce confusion and package size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsup.config.ts` around lines 1 - 20, The tsup configuration (defineConfig in tsup.config.ts) is unused because the project uses Bun.build via build.ts and tsc for declarations; delete tsup.config.ts and remove the tsup entry from package.json dependencies (ensure only dev/runtime deps remain), then run the existing build (bun run build) and CI scripts to verify nothing breaks; update any documentation or scripts referencing defineConfig/tsup if present.scripts/install-git-hooks.ts (1)
21-26: Consider adding error handling for missing source hook files.If a hook file doesn't exist at
hooksSource,Bun.file(src).text()will throw a generic error. Adding a check with a clearer error message would improve the developer experience during setup.💡 Suggested improvement
for (const name of ["pre-commit", "post-checkout"]) { const src = join(hooksSource, name); const dest = join(hooksDir, name); + const file = Bun.file(src); + if (!(await file.exists())) { + console.error(`Hook source not found: ${src}`); + process.exit(1); + } - const content = await Bun.file(src).text(); + const content = await file.text(); await writeFile(dest, content, { mode: 0o755 }); console.log(`Installed ${name} -> ${dest}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-git-hooks.ts` around lines 21 - 26, The loop that reads hook files uses Bun.file(src).text() without checking for existence, so add explicit existence/error handling around that call: before calling Bun.file(src).text() (where src is built from hooksSource and name) verify the file exists (or wrap the read in try/catch), and if missing throw or log a clear error mentioning which hook name and the hooksSource variable; on read failure include the src and underlying error in the message and avoid writing the dest via writeFile(hooksDir, name) when the source is absent or unreadable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/rewrite-core-version.ts`:
- Around line 22-23: The bug is that using "const deps = pkg.dependencies ?? {}"
creates a disconnected object when pkg.dependencies is undefined so subsequent
changes to deps won't be written back; fix by ensuring pkg.dependencies is
initialized on pkg before mutating: explicitly set pkg.dependencies =
pkg.dependencies ?? {} and then use const deps = pkg.dependencies (or mutate
pkg.dependencies directly) so modifications to deps/current are persisted when
writing the package back.
In `@src/index.test.ts`:
- Around line 41-48: The test references ModelType (used as
ModelType.TEXT_SMALL) but it is not imported, causing a ReferenceError; add an
import for ModelType from "@elizaos/core" at the top of the file (where other
imports live) so the symbol ModelType is available for the test; update
src/index.test.ts to import { ModelType } and run the test to verify the error
is resolved.
- Around line 6-25: The test uses Jest-specific APIs (jest.fn, jest.mock,
jest.spyOn) which will fail in Bun; replace them with Bun test mocks: change
jest.fn() calls for mockRuntime.emit/getSetting and mockModel and mockStreamText
to use mock() from bun:test (preserve the same mock behavior and
mockImplementation semantics), and replace the module mock jest.mock('ai', ...)
with Bun's mock.module or mockModule equivalent to stub 'ai' so streamText
returns the async generator; also convert any jest.spyOn usages (if present
elsewhere) to Bun's spy/mocking utilities so tests run under Bun. Ensure you
update symbols mockRuntime, mockModel, and mockStreamText and the module mock
for 'ai' accordingly.
---
Outside diff comments:
In `@src/index.test.ts`:
- Around line 61-65: The assertions referencing ollamaPlugin.providers are
orphaned outside any test block and cause a syntax error; either remove these
lines or move them into an existing or new it() test inside the surrounding
describe() so they run as part of a test. Locate the stray expects for
ollamaPlugin.providers in src/index.test.ts and ensure they are enclosed within
an it(...) callback (or delete them if redundant), keeping the symbols
ollamaPlugin, providers, expect, it(), and describe() in mind when placing the
assertions.
In `@src/index.ts`:
- Around line 164-199: The ensureModelAvailable function is calling the global
fetch instead of using the injected runtime.fetch (so custom timeouts/config are
bypassed); update both fetch calls inside ensureModelAvailable to use
runtime.fetch when available (e.g., runtime.fetch || fetch) so any
wrapped/fallback fetch on the runtime is honored, keeping existing request shape
and error handling; references: ensureModelAvailable, runtime.fetch,
verifiedModels, getBaseURL.
---
Nitpick comments:
In `@build.ts`:
- Around line 36-39: The current await $`tsc --project tsconfig.build.json` call
will throw on TypeScript errors and abort the build; wrap the invocation in a
try/catch (around the await $`tsc...`) so you can log a warning including the
error and continue (using process.exitCode if desired), or if strict failure is
intended, add an inline comment near dtsStart/console.log and the $`tsc
--project tsconfig.build.json` invocation explaining that type errors should
fail the build; ensure the catch logs the error and the duration calculation
that uses dtsStart still runs.
In `@package.json`:
- Line 30: The package.json still lists the unused dependency "tsup" even though
the build and dev scripts now call bun run build.ts (which uses Bun.build and
tsc directly); remove the "tsup" entry from package.json dependencies so it's no
longer installed and update any lockfile (run bun install / npm prune or
regenerate lock) to reflect the removal; ensure there are no remaining
references to "tsup" in scripts or build files (search for "tsup" and
remove/replace if found) to avoid confusion.
In `@scripts/install-git-hooks.ts`:
- Around line 21-26: The loop that reads hook files uses Bun.file(src).text()
without checking for existence, so add explicit existence/error handling around
that call: before calling Bun.file(src).text() (where src is built from
hooksSource and name) verify the file exists (or wrap the read in try/catch),
and if missing throw or log a clear error mentioning which hook name and the
hooksSource variable; on read failure include the src and underlying error in
the message and avoid writing the dest via writeFile(hooksDir, name) when the
source is absent or unreadable.
In `@tsup.config.ts`:
- Around line 1-20: The tsup configuration (defineConfig in tsup.config.ts) is
unused because the project uses Bun.build via build.ts and tsc for declarations;
delete tsup.config.ts and remove the tsup entry from package.json dependencies
(ensure only dev/runtime deps remain), then run the existing build (bun run
build) and CI scripts to verify nothing breaks; update any documentation or
scripts referencing defineConfig/tsup if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71af8742-a2c9-4c6f-ace6-fa9e2e52e57c
📒 Files selected for processing (12)
.gitignore.prr/lessons.mdREADME.mdbuild.tspackage.jsonscripts/git-hooks/post-checkoutscripts/git-hooks/pre-commitscripts/install-git-hooks.tsscripts/rewrite-core-version.tssrc/index.test.tssrc/index.tstsup.config.ts
✅ Files skipped from review due to trivial changes (1)
- .prr/lessons.md
| const deps = pkg.dependencies ?? {}; | ||
| const current = deps["@elizaos/core"]; |
There was a problem hiding this comment.
Bug: Modifications to deps won't persist if pkg.dependencies is undefined.
If pkg.dependencies is undefined, the nullish coalescing creates a new empty object that isn't connected to pkg. Changes to deps won't be reflected in the written file.
🐛 Proposed fix
-const deps = pkg.dependencies ?? {};
+pkg.dependencies = pkg.dependencies ?? {};
+const deps = pkg.dependencies;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/rewrite-core-version.ts` around lines 22 - 23, The bug is that using
"const deps = pkg.dependencies ?? {}" creates a disconnected object when
pkg.dependencies is undefined so subsequent changes to deps won't be written
back; fix by ensuring pkg.dependencies is initialized on pkg before mutating:
explicitly set pkg.dependencies = pkg.dependencies ?? {} and then use const deps
= pkg.dependencies (or mutate pkg.dependencies directly) so modifications to
deps/current are persisted when writing the package back.
| const mockRuntime = { | ||
| emit: jest.fn(), | ||
| getSetting: jest.fn().mockReturnValue(null), | ||
| }; | ||
|
|
||
| const mockModel = jest.fn().mockReturnValue({ | ||
| doStream: jest.fn(), | ||
| }); | ||
|
|
||
| const mockStreamText = jest.fn().mockImplementation(() => ({ | ||
| textStream: (async function*() { | ||
| yield 'Hello, '; | ||
| yield 'world!'; | ||
| })(), | ||
| })); | ||
|
|
||
| // Replace actual streamText with mock | ||
| jest.mock('ai', () => ({ | ||
| streamText: mockStreamText, | ||
| })); |
There was a problem hiding this comment.
Jest APIs used in Bun test file - tests will fail.
This file imports from bun:test but uses jest.fn(), jest.mock(), and jest.spyOn() which are Jest-specific APIs not available in Bun's test runtime.
Bun's test framework uses different mocking approaches:
- Use
mock()frombun:testinstead ofjest.fn() - Use
mock.module()for module mocking instead ofjest.mock()
♻️ Example fix using Bun's mocking API
-import { describe, it, expect } from 'bun:test';
+import { describe, it, expect, mock, spyOn } from 'bun:test';
import { ollamaPlugin } from './index';
import { createOllama } from 'ollama-ai-provider';
// Mocked runtime for testing
const mockRuntime = {
- emit: jest.fn(),
- getSetting: jest.fn().mockReturnValue(null),
+ emit: mock(() => {}),
+ getSetting: mock(() => null),
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.test.ts` around lines 6 - 25, The test uses Jest-specific APIs
(jest.fn, jest.mock, jest.spyOn) which will fail in Bun; replace them with Bun
test mocks: change jest.fn() calls for mockRuntime.emit/getSetting and mockModel
and mockStreamText to use mock() from bun:test (preserve the same mock behavior
and mockImplementation semantics), and replace the module mock jest.mock('ai',
...) with Bun's mock.module or mockModule equivalent to stub 'ai' so streamText
returns the async generator; also convert any jest.spyOn usages (if present
elsewhere) to Bun's spy/mocking utilities so tests run under Bun. Ensure you
update symbols mockRuntime, mockModel, and mockStreamText and the module mock
for 'ai' accordingly.
| it('should emit text stream events in order', async () => { | ||
| const result = await ollamaPlugin.models[ModelType.TEXT_SMALL](mockRuntime, { | ||
| prompt: 'Test prompt', | ||
| }); | ||
|
|
||
| expect(mockRuntime.emit).toHaveBeenCalledWith('textStream', 'Hello, '); | ||
| expect(mockRuntime.emit).toHaveBeenCalledWith('textStream', 'world!'); | ||
| expect(mockRuntime.emit).toHaveBeenCalledWith('textStreamDone', true); |
There was a problem hiding this comment.
ModelType is not imported - will cause ReferenceError.
The test references ModelType.TEXT_SMALL but ModelType is never imported. Add the import from @elizaos/core:
import { describe, it, expect } from 'bun:test';
import { ollamaPlugin } from './index';
import { createOllama } from 'ollama-ai-provider';
+import { ModelType } from '@elizaos/core';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.test.ts` around lines 41 - 48, The test references ModelType (used
as ModelType.TEXT_SMALL) but it is not imported, causing a ReferenceError; add
an import for ModelType from "@elizaos/core" at the top of the file (where other
imports live) so the symbol ModelType is available for the test; update
src/index.test.ts to import { ModelType } and run the test to verify the error
is resolved.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| expect(generateOllamaTextSpy).toHaveBeenCalled(); | ||
| expect(mockRuntime.emit).toHaveBeenCalledWith('textStreamDone', true); | ||
| }); |
There was a problem hiding this comment.
Orphaned test code creates syntax error from bad merge
High Severity
The it('should have providers array', () => { line was removed and replaced with two new test blocks, but the body of the old test (the expect calls at lines 62–63) and its closing }); at line 64 were left behind. This leaves bare expect calls outside any it() block and creates an unmatched }); that makes the file a syntax error. bun test will fail to parse this file entirely.
| if (pkg.scripts?.clean) { | ||
| console.log("🧹 Cleaning..."); | ||
| // Note: always runs clean to ensure a fresh build environment without stale artifacts | ||
| await $`bun run clean`.quiet(); |
There was a problem hiding this comment.
Build script deletes node_modules before tsc needs them
High Severity
build.ts unconditionally runs bun run clean before building. The clean script includes rm -rf node_modules, which deletes all installed dependencies. The subsequent tsc --project tsconfig.build.json step (line 38) requires node_modules to resolve type declarations for imports like @elizaos/core and ai. This causes tsc to fail or produce broken .d.ts files with unresolved types.
Additional Locations (1)
Made-with: Cursor
Note
Medium Risk
Changes the core text generation path to streaming with new runtime event semantics and new timeout/wrapper logic, which may affect model behavior and error handling in production. Build/publish automation (core version rewriting + git hooks) also increases the chance of release/process issues if misconfigured.
Overview
Switches text generation to token streaming via
streamText, emittingtextStream/textStreamDoneruntime events and updating token limit handling (maxOutputTokens). Adds AI SDK v2 compatibility shims (Proxy wrappers for language/embedding models), request timeout support for Ollama fetches, safer setting coercion, and a per-process cache to avoid repeatedly probing model availability.Reworks packaging/build/publish flow: replaces
tsupbuild scripts with a Bunbuild.ts+tscdeclarations pipeline, pins@elizaos/coreto1.7.2withpublishCoreVersionplus scripts and git hooks to rewrite/restore workspace vs published versions, and updates TS configs,.gitignore, and README publishing guidance. Adds/updates unit tests around stream event ordering and timeout behavior.Written by Cursor Bugbot for commit 7fd7803. This will update automatically on new commits. Configure here.
Greptile Summary
This PR upgrades
plugin-ollamato use streaming text generation (streamTextwithruntime.emitEvent('textStream'/'textStreamDone')), adds Proxy-based shims (wrapAsV2LanguageModel/wrapAsV2EmbeddingModel) to bridge the AI SDK v1 provider against an SDK v2 consumer, and introduces supporting infrastructure: a fetch timeout wrapper, a per-process model verification cache, agetSettingAsStringhelper, and a new Bun-basedbuild.ts.Key issues found:
process.stdout.write(delta)ingenerateOllamaText(line 212): a debug artifact that prints every streamed token to stdout in a production plugin — should be removed.AbortSignaloverride increateFetchWithTimeout: the timeout controller's signal unconditionally replaces any existinginit.signal, silently discarding caller-provided cancellation signals.devwatch mode: thebun run build.ts --watchscript passes--watchbutbuild.tsnever reads CLI arguments, so it performs a single build and exits.if (true)inbuild.ts: the hardcoded literal makes theelsetype-check-only branch permanently unreachable.tsupindependencies: a build tool that belongs indevDependencies.Confidence Score: 3/5
process.stdout.write) left in the hot path that would affect every text generation call in production.src/index.ts(debug stdout write + AbortSignal override) andbuild.ts(dead code + broken watch mode) need attention before merge.Important Files Changed
process.stdout.writeand anAbortSignaloverride bug in the timeout wrapper.if (true)leaving a dead else-branch, and the--watchflag accepted by the dev script is never parsed or acted on.@elizaos/coretoworkspace:*, replaces tsup build scripts withbun run build.ts, and changes dev to use a non-functional--watchflag;tsupremains incorrectly independenciesinstead ofdevDependencies.Sequence Diagram
sequenceDiagram participant Caller participant Plugin as ollamaPlugin (TEXT_SMALL/LARGE) participant GenText as generateOllamaText participant Timeout as createFetchWithTimeout participant OllamaProvider as ollama-ai-provider (v1 Proxy→v2) participant Ollama as Ollama API Caller->>Plugin: useModel(TEXT_SMALL | TEXT_LARGE, params) Plugin->>Timeout: wrap runtime.fetch with 600s timeout Plugin->>OllamaProvider: createOllama({ fetch: timeoutFetch, baseURL }) Plugin->>GenText: generateOllamaText(ollama, model, params, runtime) GenText->>OllamaProvider: streamText({ model: wrapAsV2LanguageModel(ollama(model)), … }) Note over OllamaProvider: Proxy overrides specificationVersion→"v2",<br/>supportedUrls→{}, injects mode.type="regular" OllamaProvider->>Ollama: POST /api/generate (streaming) loop Stream chunks Ollama-->>OllamaProvider: text delta OllamaProvider-->>GenText: delta via textStream GenText->>Caller: runtime.emitEvent("textStream", delta) end GenText->>Caller: runtime.emitEvent("textStreamDone", true) GenText-->>Plugin: joined full text Plugin-->>Caller: string resultLast reviewed commit: 926bb47
Summary by CodeRabbit
Chores
New Features
Tests
Documentation