Conversation
WalkthroughThe changes standardize error logging across multiple source files by adopting structured logging, where error objects are passed as properties within objects to logger calls. Additionally, the test suite now stores service instances in arrays within the runtime's service map. The only other change is a version bump in Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Logger
App->>Logger: logger.error({ error }, "Error message")
Note right of Logger: Structured log entry with error property
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| fn: async (runtime: IAgentRuntime) => { | ||
| const service = await KnowledgeService.start(runtime); | ||
| runtime.services.set(KnowledgeService.serviceType as any, service); | ||
| runtime.services.set(KnowledgeService.serviceType as any, [service]); |
There was a problem hiding this comment.
Bug: Service Retrieval Returns Array Instead of Object
Services are incorrectly stored as arrays in the runtime.services map. The runtime.getService method expects to retrieve a single service object, but the change causes it to return an array [service] instead. This breaks equality checks (e.g., retrievedService !== service) and subsequent calls to service methods that expect a service object.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/routes.ts (2)
202-205: Two lingering non-structured error logs — align with new patternThese still pass the error as a separate argument. Switch to structured logging for consistency.
- logger.error( - `[Document Processor] ❌ Error processing file ${file.originalname}:`, - fileError - ); + logger.error( + { error: fileError }, + `[Document Processor] ❌ Error processing file ${file.originalname}` + );
324-325: Another non-structured error log — update to structuredAlign with the rest of the file.
- logger.error(`[Document Processor] ❌ Error processing URL ${fileUrl}:`, urlError); + logger.error({ error: urlError }, `[Document Processor] ❌ Error processing URL ${fileUrl}`);
🧹 Nitpick comments (4)
package.json (1)
4-4: Patch bump looks right for a logging fixVersion 1.2.2 for a “fix:” change is consistent. Ensure CHANGELOG/release notes mention the structured logging change for consumers.
src/docs-loader.ts (1)
147-147: Structured error logging approvedSame note: verify
{ error }is the expected shape for stack/trace capture with your logger.src/llm.ts (1)
36-36: Consistent structured error loggingLGTM. Consider confirming whether
{ error }vs{ err }matters for stack serialization in your logger.src/service.ts (1)
455-457: Nit: consider omitting explicitstackif your logger already serializes ErrorSome structured loggers serialize
error.stackautomatically from theerrorobject. Duplicating it asstackmay be redundant and can increase log size.If your logger serializes Error objects, this is sufficient and more idiomatic:
- logger.error({ error, stack: error.stack }, `KnowledgeService: Error processing document ${originalFilename}`); + logger.error({ error }, `KnowledgeService: Error processing document ${originalFilename}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
package.json(1 hunks)src/actions.ts(2 hunks)src/docs-loader.ts(3 hunks)src/llm.ts(3 hunks)src/routes.ts(11 hunks)src/service.ts(12 hunks)src/tests.ts(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/service.ts (2)
src/types.ts (1)
LoadResult(166-170)src/docs-loader.ts (1)
loadDocsFromPath(36-125)
src/tests.ts (1)
src/service.ts (1)
KnowledgeService(38-904)
🔇 Additional comments (34)
src/docs-loader.ts (1)
13-14: Minor readability improvement approvedMulti-line fallback assignment is clearer without changing logic.
src/llm.ts (2)
85-85: Batch embedding: logging refactor OKChange is consistent with the new pattern.
248-248: Text generation: logging refactor OKConsistent and non-functional change as intended.
src/routes.ts (11)
77-77: Cleanup failure logging: OKStructured logging applied correctly.
338-338: Top-level processing error: OKStructured logging applied.
415-415: Retrieval error logging: OKConsistent with the new format.
449-449: Delete error logging: OKGood.
505-505: Get-by-ID error logging: OKLooks good.
564-564: Manifest read error: OKStructured logging with named variable is fine.
603-603: Frontend error logging: OKConsistent format.
650-650: Asset serving error logging: OKConsistent format.
725-725: Chunks retrieval error logging: OKConsistent format.
841-841: Search error logging: OKConsistent format.
857-857: Multer upload error logging: OKConsistent format.
src/actions.ts (2)
200-200: PROCESS_KNOWLEDGE error: structured logging OKAdopts the new pattern.
330-330: SEARCH_KNOWLEDGE error: structured logging OKConsistent with project-wide change.
src/service.ts (11)
85-87: Structured error logging LGTMUsing
logger.error({ error }, message)aligns with the new logging pattern and improves error introspection.
164-166: Info-level init logs are appropriateConsolidating success messages to
infois consistent and avoids custom levels.
171-172: Init error logging standardized
logger.error({ error }, 'Failed to initialize Knowledge plugin')matches the structured format. Good.
181-182: Background load error logging standardizedStructured error object is used; behavior unchanged. Good.
198-200: Startup character knowledge error logging standardizedStructured error object maintained; message is clear. Good.
333-335: PDF base64 decode error: structured logging LGTMIncludes filename context; the thrown error remains informative. Good.
346-348: Binary base64 decode error: structured logging LGTMSame pattern and clarity as the PDF branch. Good.
378-379: Text base64 decode error: structured logging LGTMConsistent shape
{ error }with message; thrown error preserves user-facing clarity.
465-466: Centralized error handler logging standardized
handleProcessingErrornow emits structured logs; good centralization.
800-802: Fragment processing error logging standardizedConsistent
{ error }payload with document/fragment context in the message. Good.
819-820: processDocumentFragment error logging standardizedStructured error with fragment id; rethrows to propagate failure. Good.
src/tests.ts (7)
638-639: Consistent with new array shapeThis registration is fine once the mock runtime stores arrays as proposed above.
678-679: Consistent with new array shapeSame note as above; safe after mock runtime refactor.
715-716: Consistent with new array shapeRegistration will be correct once the services map stores arrays and getService returns the first entry.
766-766: Key alias also aligned with array registrySetting under literal key 'knowledge' as an array is fine post-refactor. Ensure any code reading directly from
runtime.servicesexpects arrays.
889-889: Consistent with new array shapeNo issues after updating the mock runtime to arrays. Prior to that, this would be a type violation.
955-957: Dual registration under both keys as arrays is fineRegistering under
KnowledgeService.serviceTypeand'knowledge'matches mixed lookup usage. Works correctly with the array-based mock runtime.
1032-1033: Consistent with new array shapeLarge document test registration OK once the mock services map stores arrays.
| successful++; | ||
| } catch (error) { | ||
| logger.error(`Failed to process file ${filePath}:`, error); | ||
| logger.error({ error }, `Failed to process file ${filePath}`); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Structured error logging adopted here
Good switch to structured error logging. Confirm that the logger recognizes the "error" key for proper stack capture (some loggers expect an Error or { err }).
Run this quick check to spot non-structured usages still present and ensure consistency:
🏁 Script executed:
#!/bin/bash
# Count structured vs. likely unstructured logger.error calls
echo "Structured (object with error):"
rg -n "logger\.error\(\s*\{\s*error\s*:" -g "src/**" || true
rg -n "logger\.error\(\s*\{\s*error\s*\}" -g "src/**" || true
echo -e "\nPossibly unstructured (message, errorVar):"
rg -n "logger\.error\([^)]*`[^`]*`[^)]*,\s*[a-zA-Z_][a-zA-Z0-9_]*\s*\)" -g "src/**" || true
rg -n "logger\.error\([^)]*'[^']*'[^)]*,\s*[a-zA-Z_][a-zA-Z0-9_]*\s*\)" -g "src/**" || trueLength of output: 2720
🏁 Script executed:
#!/bin/bash
# List all logger.error calls to establish baseline
echo "All logger.error calls:"
rg -n "logger\.error" -g "src/**" || true
# Check for unstructured usage: logger.error('message', errorVar) or logger.error("message", errorVar)
echo -e "\nPotential unstructured logger.error calls (single/double quotes):"
rg -n "logger\.error\(\s*['\"].*['\"],\s*[a-zA-Z_]" -g "src/**" || true
# Check for unstructured usage with template literals: logger.error(`message`, errorVar)
echo -e "\nPotential unstructured logger.error calls (backticks):"
rg -n "logger\.error\(\s*\`.*\`,\s*[a-zA-Z_]" -g "src/**" || trueLength of output: 4824
Ensure consistent structured error logging
We still have a few logger.error calls using the old (message, errorVar) signature, which won’t capture stacks under our new structured‐logging setup. Please convert these to pass the error object under an error key:
• src/routes.ts:324
- logger.error(`[Document Processor] ❌ Error processing URL ${fileUrl}:`, urlError);
+ logger.error({ error: urlError }, `[Document Processor] ❌ Error processing URL ${fileUrl}:`);• src/utils.ts:172
- logger.error(`[PdfService] Error converting PDF ${docName}:`, error.message);
+ logger.error({ error }, `[PdfService] Error converting PDF ${docName}:`);After updating these, run a global check (e.g. rg "logger\\.error" -g "src/**" | grep -v "{ error") to catch any remaining unstructured calls. Also verify your logger implementation picks up the error key for stack traces.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/docs-loader.ts at line 111, the logger.error call uses the old signature
that does not pass the error object under an error key, which prevents capturing
stack traces properly. Update this call to use structured logging by passing the
error object inside an object with the key error, like logger.error({ error },
`Failed to process file ${filePath}`). After fixing this, review the other
specified files src/routes.ts line 324 and src/utils.ts line 172 for similar
issues and update them accordingly. Finally, run a global search to find any
remaining unstructured logger.error calls and ensure your logger implementation
correctly handles the error key for stack traces.
| const result: LoadResult = await loadDocsFromPath( | ||
| this as any, | ||
| this as any, | ||
| this.runtime.agentId, | ||
| undefined, // worldId | ||
| knowledgePath | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Drop the any cast; use type-only import in docs-loader to keep strong typing
Passing this as any masks real type issues and weakens safety. The likely cause is a type/runtime cycle with docs-loader.ts. Prefer a type-only import in docs-loader and pass this directly.
Proposed change here:
- const result: LoadResult = await loadDocsFromPath(
- this as any,
+ const result: LoadResult = await loadDocsFromPath(
+ this,
this.runtime.agentId,
undefined, // worldId
knowledgePath
);And in src/docs-loader.ts (outside this diff), ensure the KnowledgeService type is imported type-only to avoid runtime cycles:
// at top of docs-loader.ts
import type { KnowledgeService } from './service.ts';Alternatively, decouple the function signature to accept a narrow interface and avoid referencing the class type:
// docs-loader.ts
export type KnowledgeAdder = (opts: AddKnowledgeOptions) => Promise<{ fragmentCount: number }>;
export async function loadDocsFromPath(
service: { addKnowledge: KnowledgeAdder },
agentId: UUID,
worldId?: UUID,
knowledgePath?: string
) { /* ... */ }Please confirm docs-loader.ts uses a type-only import or a narrowed interface so we can safely remove the any cast.
I’ll check how loadDocsFromPath is defined and whether docs-loader.ts already uses a type-only import.
🏁 Script executed:
#!/bin/bash
set -e
# Show the declaration of loadDocsFromPath in docs-loader.ts
echo "=== loadDocsFromPath Declaration ==="
rg --context 3 --max-count 1 "export async function loadDocsFromPath" -n src/docs-loader.ts
# Show the top of docs-loader.ts to inspect imports
echo
echo "=== Top of docs-loader.ts ==="
head -n 20 src/docs-loader.tsLength of output: 1322
Use a type-only import in docs-loader and remove the any cast
To preserve strong typing and avoid runtime cycles, update both files:
• src/service.ts (around the loadDocsFromPath call)
• src/docs-loader.ts (at the top import)
Proposed diffs:
--- a/src/service.ts
+++ b/src/service.ts
@@ -67,7 +67,7 @@
const result: LoadResult = await loadDocsFromPath(
- this as any,
+ this,
this.runtime.agentId,
undefined, // worldId
knowledgePath
);--- a/src/docs-loader.ts
+++ b/src/docs-loader.ts
@@ -3,7 +3,7 @@
import { logger, UUID } from '@elizaos/core';
import * as fs from 'fs';
import * as path from 'path';
-import { KnowledgeService } from './service.ts';
+import type { KnowledgeService } from './service.ts';
import { AddKnowledgeOptions } from './types.ts';
import { isBinaryContentType } from './utils.ts';These changes let you drop the any cast safely while keeping KnowledgeService purely a compile-time type.
🤖 Prompt for AI Agents
In src/service.ts around lines 67 to 72, the call to loadDocsFromPath casts
'this' to 'any', which weakens type safety and may cause runtime issues. To fix
this, update the import of KnowledgeService in src/docs-loader.ts to be a
type-only import, ensuring it is used only for type checking and does not create
runtime dependencies. Then, remove the 'any' cast in src/service.ts and pass
'this' directly with the correct type, preserving strong typing and avoiding
runtime cycles.
| runtime.services.set(KnowledgeService.serviceType as any, [service]); | ||
| const retrievedService = runtime.getService(KnowledgeService.serviceType); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Array-based service registry requires mock runtime updates
You’re storing [service] under the key, but the mock defines services: Map<string, Service> and getService returns a single Service. The lifecycle test that compares retrievedService !== service will fail because getService returns the array.
Update the mock to hold arrays and make getService return the first service:
- const services: Map<string, Service> = new Map();
+ const services: Map<string, Service[]> = new Map();- getService<T extends Service>(name: string): T | null {
- return (services.get(name) as T) || null;
- },
+ getService<T extends Service>(name: string): T | null {
+ const entry = services.get(name);
+ return (entry?.[0] as T) || null;
+ },- getAllServices() {
- return services;
- },
+ getAllServices() {
+ return services; // Map<string, Service[]>
+ },- async registerService(ServiceClass: typeof Service) {
- const service = await ServiceClass.start(this);
- services.set(ServiceClass.serviceType, service);
- },
+ async registerService(ServiceClass: typeof Service) {
+ const service = await ServiceClass.start(this);
+ const key = ServiceClass.serviceType;
+ const list = services.get(key) ?? [];
+ list.push(service);
+ services.set(key, list);
+ },After applying the above, the equality check in this test should succeed and the other tests that do runtime.services.set(..., [service]) will be consistent with the mock.
Let’s locate the mock runtime implementation to confirm its current shape before making refactors.
🏁 Script executed:
#!/bin/bash
# Find where the services Map is declared
rg -n "const services" -A3
# Find getService definition
rg -n "getService<.*>" -A5
# Find getAllServices
rg -n "getAllServices" -A3
# Find registerService implementation
rg -n "registerService" -A5Length of output: 3500
Refactor mock runtime to support array-based service registry
The mock in src/tests.ts still uses Map<string, Service> and single‐item APIs, but tests now store arrays ([service]) and expect getService to unwrap the first element. Update these four spots in src/tests.ts:
• Change the services declaration to hold an array
• Make getService return the first entry of that array
• Update getAllServices’ return type to Map<string, Service[]>
• Adjust registerService to append to the array instead of replacing
--- a/src/tests.ts
+++ b/src/tests.ts
@@ 92c92
- const services: Map<string, Service> = new Map();
+ const services: Map<string, Service[]> = new Map();
@@ 331,333c331,335
- getService<T extends Service>(name: string): T | null {
- return (services.get(name) as T) || null;
- },
+ getService<T extends Service>(name: string): T | null {
+ const entry = services.get(name);
+ return (entry?.[0] as T) || null;
+ },
@@ 335,337c337,338
- getAllServices() {
- return services;
- },
+ getAllServices(): Map<string, Service[]> {
+ return services;
+ },
@@ 339,342c340,345
- async registerService(ServiceClass: typeof Service) {
- const service = await ServiceClass.start(this);
- services.set(ServiceClass.serviceType, service);
- },
+ async registerService(ServiceClass: typeof Service) {
+ const service = await ServiceClass.start(this);
+ const key = ServiceClass.serviceType;
+ const list = services.get(key) ?? [];
+ list.push(service);
+ services.set(key, list);
+ },After applying these changes, your lifecycle test’s equality check (retrievedService === service) will pass, and any code/tests that do runtime.services.set(..., [service]) remain consistent.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/tests.ts around lines 558 to 560, refactor the mock runtime's service
registry to use arrays instead of single service instances. Change the services
declaration to Map<string, Service[]> to hold arrays, modify getService to
return the first element of the array for a given key, update getAllServices to
reflect the new Map<string, Service[]> type, and adjust registerService to
append new services to the existing array rather than replacing it. This will
ensure consistency with tests storing arrays and fix the equality check in
lifecycle tests.
1 participant
Fixing logger issue.
Summary by CodeRabbit
Style
Chores
Tests