fix: use state.values.recentMessages for cloud compatibility#12
fix: use state.values.recentMessages for cloud compatibility#12
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
standujar
force-pushed
the
fix/conversation-context-state-path
branch
from
ddd31ac to
3344a94
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/context.ts`:
- Around line 3-10: buildConversationContext can crash when message.content is
undefined; compute a single safe fallback string like const text =
message.content?.text ?? '' at the top of the function and use that variable in
both the empty-case return and the final template return instead of directly
accessing message.content.text, keeping the rest of the logic (recentMessages
check and template) unchanged.
3344a94 to
4eec9e8
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
src/utils/context.ts
Outdated
| ): string { | ||
| const recentMessages = (state?.data?.recentMessages as Memory[]) || []; | ||
| export function buildConversationContext(message: Memory, state: State | undefined): string { | ||
| const recentMessages = state?.values?.recentMessages as string; |
There was a problem hiding this comment.
Type Safety Issue: This as string assertion is unsafe. state?.values?.recentMessages could be undefined, a number, an object, or any other type. This will cause runtime failures if the provider passes an unexpected type.
| const recentMessages = state?.values?.recentMessages as string; | |
| const recentMessages = state?.values?.recentMessages; | |
| if (!recentMessages || typeof recentMessages !== 'string') { |
Also consider adding runtime validation or defensive type checking.
Critical Issue: Incomplete MigrationThis PR has a breaking change that will cause TypeScript compilation errors and runtime failures in other parts of the codebase. ProblemThe function signature was changed from: buildConversationContext(runtime: IAgentRuntime, message: Memory, state: State | undefined)To: buildConversationContext(message: Memory, state: State | undefined)However, only one call site was updated (createWorkflow.ts:272). The following files still call it with the old signature:
ImpactThis will cause:
Required FixBefore merging, you must update all call sites to use the new signature: // OLD (will break)
const context = buildConversationContext(runtime, message, state);
// NEW (correct)
const context = buildConversationContext(message, state);RecommendationRun |
| } from "../helpers/mockRuntime"; | ||
| import { describe, test, expect } from 'bun:test'; | ||
| import { buildConversationContext } from '../../src/utils/context'; | ||
| import { createMockMessage, createMockState } from '../helpers/mockRuntime'; |
There was a problem hiding this comment.
Unused Import: createMockRuntime is no longer imported but the test file doesn't need it anymore since the runtime parameter was removed. Good cleanup!
| entityId: "user-001", | ||
| content: { text: `Message ${i}` }, | ||
| })); | ||
| test('preserves recentMessages formatting from provider', () => { |
There was a problem hiding this comment.
Missing Test Coverage: While this test verifies that preformatted messages are preserved, there's no test coverage for:
- Non-string values in
state.values.recentMessages- What happens if it's a number, object, or array? Theas stringassertion on line 4 ofcontext.tswill silently coerce these. - Edge cases - Empty string, whitespace-only strings, very long strings, strings with special characters/unicode
Consider adding tests like:
test('handles non-string recentMessages gracefully', () => {
const message = createMockMessage({ content: { text: 'Test' } });
const state = createMockState({
values: { recentMessages: 123 }, // number instead of string
});
const result = buildConversationContext(message, state);
expect(result).toBe('Test'); // Should fall back to message text
});
Security & Code Quality ReviewSummaryThis is a well-structured ElizaOS plugin for n8n workflow automation. The code demonstrates good TypeScript practices and comprehensive test coverage. However, I've identified several critical security issues and code quality concerns that should be addressed before merging. Critical Security Issues1. API Key Exposure in LogsLocation: src/index.ts:74 The plugin logs whether the API key is configured. This could leak sensitive information in production environments. Risk: If logging is verbose or logs are accessible to unauthorized users, this reveals security configuration details. Recommendation: Remove API key status from logs entirely, or only log at debug level in development. 2. Unvalidated User Input in Workflow NamesLocations: src/utils/generation.ts:250, src/actions/createWorkflow.ts The workflow name field accepts arbitrary user input without sanitization, which could lead to XSS vulnerabilities, injection attacks, or log injection. Recommendation: Implement strict input validation and sanitization for workflow names with length limits and character filtering. 3. Missing Rate LimitingLocation: src/utils/api.ts The API client has no rate limiting or retry logic. This could lead to DoS vulnerabilities, service degradation, or account suspension from n8n API. Recommendation: Implement exponential backoff and rate limiting. 4. Credential Data HandlingLocation: src/utils/credentialResolver.ts:108-112 Credential data from external providers is directly passed to the n8n API without validation. Risk: If the external provider is compromised or malicious, it could inject arbitrary data into n8n credentials. Recommendation: Validate credential data structure before passing to API. High Priority Issues5. Type Safety: Unsafe Type AssertionsLocation: src/utils/api.ts:287-288 Unsafe type assertion returning undefined as T violates TypeScript type safety. Recommendation: Use proper type guards or modify return type. 6. Error Handling: Silent FailuresLocations: src/utils/credentialResolver.ts:115-120, src/services/n8n-workflow-service.ts:375-379 Cache failures and workflow tagging failures are logged but swallowed. Recommendation: Consider whether these failures should propagate or be tracked in metrics. 7. Potential Race Condition in Draft CacheLocation: src/actions/createWorkflow.ts:274-281 The draft cache TTL check and delete are not atomic, which could cause race conditions. Recommendation: Use atomic operations or add mutex locking. 8. Input Validation for Database OperationsLocation: src/services/n8n-credential-store.ts:49-56 userId from message.entityId should be validated before database queries. Recommendation: Add input validation for all database query parameters. Code Quality Improvements
Testing RecommendationsConsider adding:
Overall AssessmentThis is a well-architected plugin with good separation of concerns and comprehensive functionality. The RAG pipeline for workflow generation is innovative. With the security fixes applied, this will be production-ready. Estimated effort to address critical issues: 4-6 hours See inline comments for specific code issues. |
Additional Code Quality Issues1. Type Safety ConcernsLocation: The code uses an unsafe type assertion: const recentMessages = state?.values?.recentMessages as string;This assumes Recommendation: Add runtime type checking: const recentMessages = state?.values?.recentMessages;
if (\!recentMessages || typeof recentMessages \!== 'string') {
return message.content.text || '';
}2. Test Coverage GapsThe new tests cover the happy path but miss edge cases:
3. Missing DocumentationThe PR changes the fundamental contract of
Consider adding: /**
* Builds conversation context for workflow actions.
*
* @param message - The current user message
* @param state - Optional state containing recentMessages from the provider
* @returns Formatted conversation context with current request appended
*
* @remarks
* Expects state.values.recentMessages to be a pre-formatted string from the provider.
* Falls back to message.content.text if recentMessages is unavailable.
*/4. Positive Notes ✓
|
Security ReviewNo Critical Security Issues Found ✓I reviewed the changes for common security vulnerabilities:
Minor Security ConsiderationThe reliance on
However, this is an existing risk not introduced by this PR. The change actually reduces complexity by delegating formatting to the provider. SummaryCANNOT MERGE ❌ - Contains breaking changes that will prevent compilation. Must Fix Before Merge:
Recommended:
|
standujar
force-pushed
the
fix/conversation-context-state-path
branch
from
4eec9e8 to
2df841c
Compare
| state: State | undefined | ||
| ): string { | ||
| const recentMessages = (state?.data?.recentMessages as Memory[]) || []; | ||
| export function buildConversationContext(message: Memory, state: State | undefined): string { |
There was a problem hiding this comment.
Breaking Change Alert: Removing the runtime parameter is a breaking change that affects the function signature. Ensure all callers have been updated.
Code Quality: The change from processing individual messages to receiving pre-formatted strings is good for simplification, but consider:
- Adding JSDoc to document the expected format of
recentMessages - Adding validation to ensure
recentMessagesis a string if present
src/utils/context.ts
Outdated
| ): string { | ||
| const recentMessages = (state?.data?.recentMessages as Memory[]) || []; | ||
| export function buildConversationContext(message: Memory, state: State | undefined): string { | ||
| const recentMessages = state?.values?.recentMessages as string; |
There was a problem hiding this comment.
Type Safety Issue: Casting state?.values?.recentMessages as string without validation is unsafe. If the value is not a string (e.g., accidentally set to an object or array), this will cause runtime errors.
Recommendation: Add type validation:
const recentMessages = state?.values?.recentMessages;
if (recentMessages && typeof recentMessages !== 'string') {
logger.warn('recentMessages is not a string, ignoring');
return message.content.text || '';
}
| if (!recentMessages) { | ||
| return message.content.text || ''; | ||
| } | ||
|
|
There was a problem hiding this comment.
Security Consideration: The pre-formatted recentMessages string is now inserted directly into the context without any sanitization. If this context is used in prompts to LLMs, ensure that:
- The provider that creates
recentMessagesproperly sanitizes input - There's no risk of prompt injection through user-controlled message content
- Message length is bounded to prevent context overflow
Recommendation: Add length validation:
if (recentMessages.length > 10000) { // reasonable limit
return `${recentMessages.slice(-10000)}\n\nCurrent request: ${message.content.text || ''}`;
}
| test("includes recent messages in context", () => { | ||
| const runtime = createMockRuntime({ agentId: "agent-001" }); | ||
| const message = createMockMessage({ content: { text: "Activate it" } }); | ||
| test('appends current request to recentMessages', () => { |
There was a problem hiding this comment.
Test Coverage: The tests now validate the new string-based format, which is good. However, consider adding tests for:
- Malformed input: What happens if
recentMessagesis not a string? - Very long messages: Test behavior with extremely long
recentMessagesstrings - Special characters: Test with messages containing newlines, control characters, or other special chars
- Empty strings vs undefined: Test the difference between
recentMessages: ''andrecentMessages: undefined
These edge cases are important for robustness in production.
standujar
force-pushed
the
fix/conversation-context-state-path
branch
from
2df841c to
3b3cd55
Compare
Code Review SummaryThis PR fixes a critical cloud compatibility issue by updating ✅ Positive Points
|
Code Review SummaryI've reviewed PR #12 which fixes conversation context handling for cloud compatibility. The changes look good overall, but I've identified several items worth discussing. ✅ Positive Changes
|
src/utils/context.ts
Outdated
| ): string { | ||
| const recentMessages = (state?.data?.recentMessages as Memory[]) || []; | ||
| export function buildConversationContext(message: Memory, state: State | undefined): string { | ||
| const recentMessages = state?.values?.recentMessages as string; |
There was a problem hiding this comment.
Type Safety Issue: The as string type assertion is unsafe. If a provider supplies recentMessages as a non-string type (array, object, etc.), this will fail silently or cause runtime errors when used in template literals.
Recommendation: Add runtime type checking:
const recentMessages = state?.values?.recentMessages;
if (typeof recentMessages !== 'string' || !recentMessages) {
return message.content.text || '';
}Or use a type guard for better safety.
src/utils/context.ts
Outdated
| .join('\n'); | ||
|
|
||
| return `Recent conversation:\n${context}\n\nCurrent request: ${message.content.text || ''}`; | ||
| return `${recentMessages}\n\nCurrent request: ${message.content.text || ''}`; |
There was a problem hiding this comment.
Potential Runtime Error: If message.content is undefined or null, accessing message.content.text will throw a TypeError.
Recommendation: Use optional chaining:
return `${recentMessages}\n\nCurrent request: ${message.content?.text || ''}`;This matches the defensive pattern used in line 7.
| state: State | undefined | ||
| ): string { | ||
| const recentMessages = (state?.data?.recentMessages as Memory[]) || []; | ||
| export function buildConversationContext(message: Memory, state: State | undefined): string { |
There was a problem hiding this comment.
Good refactoring: Removing the unused runtime parameter simplifies the API and reduces coupling. The function signature is now cleaner and more focused.
src/utils/context.ts
Outdated
| ): string { | ||
| const recentMessages = (state?.data?.recentMessages as Memory[]) || []; | ||
| export function buildConversationContext(message: Memory, state: State | undefined): string { | ||
| const recentMessages = state?.values?.recentMessages as string; |
There was a problem hiding this comment.
Type Safety Concern: Casting to string without validation could cause runtime issues if a provider sets this to a non-string value.
Consider adding a runtime type check:
const recentMessages = state?.values?.recentMessages;
if (recentMessages && typeof recentMessages !== 'string') {
// Log warning or handle gracefully
return message.content.text || '';
}This would make the function more defensive against unexpected provider implementations.
| @@ -1,22 +1,13 @@ | |||
| import { type IAgentRuntime, type Memory, type State, type UUID } from '@elizaos/core'; | |||
There was a problem hiding this comment.
Unused Import: IAgentRuntime and UUID are no longer used after removing the runtime parameter. Consider cleaning up:
import { type Memory, type State } from '@elizaos/core';
| entityId: "user-001", | ||
| content: { text: `Message ${i}` }, | ||
| })); | ||
| test('preserves recentMessages formatting from provider', () => { |
There was a problem hiding this comment.
Good test coverage: This test verifies that the function preserves whatever formatting the provider supplies, which is exactly the right behavior. This demonstrates the simplified approach works correctly with different provider implementations.
standujar
force-pushed
the
fix/conversation-context-state-path
branch
from
3b3cd55 to
f89200f
Compare
src/utils/context.ts
Outdated
| .join('\n'); | ||
|
|
||
| return `Recent conversation:\n${context}\n\nCurrent request: ${message.content.text || ''}`; | ||
| return `${recentMessages}\n\nCurrent request: ${message.content.text || ''}`; |
There was a problem hiding this comment.
Edge Case: If message.content.text is empty and recentMessages exists, this will result in a string ending with "\n\nCurrent request: ".
Consider:
const currentText = message.content.text || '';
if (!currentText) {
return recentMessages; // Don't append empty "Current request:"
}
return `${recentMessages}\n\nCurrent request: ${currentText}`;This would avoid awkward formatting when there's no current request text.
| const result = buildConversationContext(message, state); | ||
| expect(result).toBe(`${preformattedMessages}\n\nCurrent request: Do something`); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage: Consider adding tests for edge cases:
- Non-string recentMessages: What happens if a provider mistakenly provides an array or object?
test('handles non-string recentMessages gracefully', () => {
const message = createMockMessage({ content: { text: 'Test' } });
const state = createMockState({
values: { recentMessages: ['msg1', 'msg2'] }, // Wrong type
});
const result = buildConversationContext(message, state);
expect(result).toBe('Test'); // Should fallback safely
});- Undefined message.content: Verify it doesn't throw
test('handles undefined message content', () => {
const message = createMockMessage({ content: undefined });
const state = createMockState();
expect(() => buildConversationContext(message, state)).not.toThrow();
});
| test("returns message text when no recent messages", () => { | ||
| const runtime = createMockRuntime(); | ||
| describe('buildConversationContext', () => { | ||
| test('returns message text when no recent messages in values', () => { |
There was a problem hiding this comment.
Test Coverage Gap: The old tests validated important behaviors that are now gone:
- Message limiting (5 messages max) - removed in the refactor but not explicitly verified to be unnecessary
- Role labeling (User vs Assistant) - now delegated to provider, but should we test the provider contract?
Consider adding a test that verifies behavior when state.values.recentMessages is set to an unexpected type (number, object, etc.) to ensure graceful degradation.
| entityId: "user-001", | ||
| content: { text: `Message ${i}` }, | ||
| })); | ||
| test('preserves recentMessages formatting from provider', () => { |
There was a problem hiding this comment.
Good Test: This test validates that pre-formatted messages from providers are preserved as-is, which is important for the cloud compatibility fix. Nice addition! 👍
However, the test name "preserves recentMessages formatting from provider" could be more specific about what formatting is being preserved (timestamps, custom prefixes, etc.).
| const userId = message.entityId; | ||
| const cacheKey = `workflow_draft:${userId}`; | ||
| const generationContext = buildConversationContext(runtime, message, state); | ||
| const generationContext = buildConversationContext(message, state); |
There was a problem hiding this comment.
Consistent Update: ✅ Correctly updated to use the new 2-parameter signature. This is the critical path where conversation context is used for workflow generation, so the fix here ensures the LLM gets proper context on both cloud and bootstrap providers.
| { | ||
| "name": "@elizaos/plugin-n8n-workflow", | ||
| "version": "1.0.10", | ||
| "version": "1.0.11", |
There was a problem hiding this comment.
Version Bump: ✅ Appropriate patch version bump (1.0.10 → 1.0.11) for a bug fix. This follows semantic versioning correctly.
Additional Context: Security & Architecture Analysis🔒 Security AssessmentNo new security vulnerabilities introduced. The changes are minimal and focused:
The type safety issue mentioned earlier (unsafe 🏗️ Architecture NotesMigration Path Analysis:
Provider Compatibility:
This is the right abstraction level - letting providers format messages rather than re-formatting them. 📊 Code Quality Metrics
✅ Final VerdictApprove with minor improvements recommended. The critical bug is fixed, tests pass, and the refactoring improves code quality. Address the type safety issue for production robustness. |
| const recentMessages = (state?.data?.recentMessages as Memory[]) || []; | ||
| export function buildConversationContext(message: Memory, state: State | undefined): string { | ||
| const raw = state?.values?.recentMessages; | ||
| const recentMessages = typeof raw === 'string' ? raw : ''; |
There was a problem hiding this comment.
Type Safety Issue: This type check is too narrow. If state.values.recentMessages is not a string (e.g., an array, object, or number), it will be coerced to an empty string, potentially hiding data issues.
Consider explicit validation:
| const recentMessages = typeof raw === 'string' ? raw : ''; | |
| const raw = state?.values?.recentMessages; | |
| if (typeof raw !== 'string') { | |
| return message.content.text || ''; | |
| } | |
| const recentMessages = raw; |
This would make unexpected types fail fast during development rather than silently converting to empty string.
src/utils/context.ts
Outdated
| .join('\n'); | ||
|
|
||
| return `Recent conversation:\n${context}\n\nCurrent request: ${message.content.text || ''}`; | ||
| return `${recentMessages}\n\nCurrent request: ${message.content.text || ''}`; |
There was a problem hiding this comment.
Potential Issue: If message.content.text is falsy but recentMessages exists, this returns just the recent messages without "Current request:" prefix. This could confuse the LLM context.
Consider:
| return `${recentMessages}\n\nCurrent request: ${message.content.text || ''}`; | |
| const currentRequest = message.content.text || ''; | |
| return currentRequest | |
| ? `${recentMessages}\n\nCurrent request: ${currentRequest}` | |
| : recentMessages; |
This makes the behavior explicit and avoids an awkward "Current request: " with empty text.
| const result = buildConversationContext(message, state); | ||
| expect(result).toBe(`${preformattedMessages}\n\nCurrent request: Do something`); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing Test Coverage: The tests don't cover important edge cases:
- What happens if
state.values.recentMessagesis not a string (e.g., array, number, object)? - What happens if
message.content.textis falsy butrecentMessagesexists? - What happens if
recentMessagesis a very long string (potential token/context issues)?
Consider adding tests like:
test('handles non-string recentMessages gracefully', () => {
const message = createMockMessage({ content: { text: 'Request' } });
const state = createMockState({
values: { recentMessages: ['msg1', 'msg2'] }, // Array instead of string
});
const result = buildConversationContext(message, state);
expect(result).toBe('Request'); // Should fallback safely
});
test('handles empty current message with recentMessages', () => {
const message = createMockMessage({ content: { text: '' } });
const state = createMockState({
values: { recentMessages: 'User: Previous message' },
});
const result = buildConversationContext(message, state);
// Verify expected behavior for this edge case
});
standujar
force-pushed
the
fix/conversation-context-state-path
branch
from
e7c44b7 to
6b5c542
Compare
Code Review SummaryThis PR fixes a critical cloud compatibility issue by switching from ✅ Positive Changes
|
standujar
force-pushed
the
fix/conversation-context-state-path
branch
from
6b5c542 to
cfb87ce
Compare
Code Review SummaryOverall Assessment: The fix correctly addresses the cloud compatibility issue by switching from Positive Changes:
Issues Identified:
Recommendations:
Security: No security concerns identified. The change actually improves security by preventing potential null reference errors. |
| export function buildConversationContext(message: Memory, state: State | undefined): string { | ||
| const raw = state?.values?.recentMessages; | ||
| const recentMessages = typeof raw === 'string' ? raw : ''; | ||
| const currentText = message.content?.text ?? ''; |
There was a problem hiding this comment.
Type Safety Issue: message.content could be undefined according to the Memory type from @elizaos/core. This would cause message.content?.text to be evaluated but message.content.text is accessed without the optional chaining on line 6.
| const currentText = message.content?.text ?? ''; | |
| const currentText = (message.content?.text ?? '') as string; |
This ensures type safety even if message.content is undefined.
|
|
||
| if (recentMessages.length === 0) { | ||
| return message.content.text || ''; | ||
| if (!recentMessages) { |
There was a problem hiding this comment.
Potential Logic Bug: This condition treats an empty string "" as falsy and returns only currentText. This means if the provider returns an empty string for recentMessages (instead of undefined), the context won't include "Current request:" prefix.
Consider making this check more explicit:
| if (!recentMessages) { | |
| if (recentMessages === '' || recentMessages === undefined) { |
Or alternatively, if empty string should be treated the same as missing:
| if (!recentMessages) { | |
| if (!recentMessages || recentMessages.trim() === '') { |
This makes the intention clearer and prevents potential bugs with different providers.
Code Review SummaryI've reviewed this PR which fixes cloud compatibility by switching from ✅ Positive Changes
Issues FoundI've posted inline comments on specific concerns:
SecurityNo security issues identified. The changes don't introduce any new attack vectors and properly handle user input. Recommendations
|
| test("handles undefined state", () => { | ||
| const runtime = createMockRuntime(); | ||
| const message = createMockMessage({ content: { text: "Hello" } }); | ||
| test('handles undefined state', () => { |
There was a problem hiding this comment.
Missing Edge Case Test: Consider adding a test for when message.content itself is undefined, not just when content.text is empty:
test('handles undefined message content', () => {
const message = createMockMessage({ content: undefined });
const state = createMockState();
const result = buildConversationContext(message, undefined);
expect(result).toBe('');
});This would ensure the type safety issue mentioned in context.ts:6 is covered.
| export function buildConversationContext(message: Memory, state: State | undefined): string { | ||
| const raw = state?.values?.recentMessages; | ||
| const recentMessages = typeof raw === 'string' ? raw : ''; | ||
| const currentText = message.content?.text ?? ''; |
There was a problem hiding this comment.
Type Safety Concern: The use of message.content?.text with optional chaining is good, but message.content could potentially be undefined in the type system. Consider adding a guard or making the type contract more explicit.
However, looking at the usage in the codebase, all callers cast to Content type first (e.g., message.content as Content in createWorkflow.ts:268), so this is defensive programming which is good practice.
| test("includes recent messages in context", () => { | ||
| const runtime = createMockRuntime({ agentId: "agent-001" }); | ||
| const message = createMockMessage({ content: { text: "Activate it" } }); | ||
| test('appends current request to recentMessages', () => { |
There was a problem hiding this comment.
Test Coverage Gap: Consider adding a test for when state.values.recentMessages is an empty string (not undefined):
test('handles empty string recentMessages', () => {
const message = createMockMessage({ content: { text: 'Test' } });
const state = createMockState({
values: { recentMessages: '' },
});
const result = buildConversationContext(message, state);
expect(result).toBe('Test');
});This would clarify the expected behavior for the falsy check on line 8 of context.ts.
| ): string { | ||
| const recentMessages = (state?.data?.recentMessages as Memory[]) || []; | ||
| export function buildConversationContext(message: Memory, state: State | undefined): string { | ||
| const raw = state?.values?.recentMessages; |
There was a problem hiding this comment.
Edge Case - Type Safety: While typeof raw === 'string' is a good runtime check, consider what happens if state.values.recentMessages exists but is not a string (e.g., it's an object or array due to a provider bug or version mismatch).
Current behavior: Falls back to empty string, which is safe but silent. Consider logging a warning if the value exists but has an unexpected type:
const raw = state?.values?.recentMessages;
if (raw !== undefined && typeof raw !== 'string') {
logger.warn('state.values.recentMessages has unexpected type:', typeof raw);
}
const recentMessages = typeof raw === 'string' ? raw : '';This would help catch provider contract violations during development.
| ): string { | ||
| const recentMessages = (state?.data?.recentMessages as Memory[]) || []; | ||
| export function buildConversationContext(message: Memory, state: State | undefined): string { | ||
| const raw = state?.values?.recentMessages; |
There was a problem hiding this comment.
TypeScript Best Practice: The type check here is good, but consider using a type guard for better type safety throughout the function:
const raw = state?.values?.recentMessages;
const recentMessages = typeof raw === 'string' ? raw : '';This is already implemented correctly. Good defensive coding! ✅
However, if state.values.recentMessages could legitimately be other types (number, object, etc.), you might want to add logging when it's not a string to help debug configuration issues in different providers.
|
|
||
| if (recentMessages.length === 0) { | ||
| return message.content.text || ''; | ||
| if (!recentMessages) { |
There was a problem hiding this comment.
Logic Question: Should an empty string be treated the same as undefined?
Currently, if (!recentMessages) will be true for both undefined and '' (empty string). This means if a provider sets state.values.recentMessages = '', you'd return just the current text without the "Current request:" prefix.
Is this intended behavior? If so, it's fine. If not, consider:
if (!recentMessages || recentMessages.length === 0) {or be explicit about what you're checking.
| const userId = message.entityId; | ||
| const cacheKey = `workflow_draft:${userId}`; | ||
| const generationContext = buildConversationContext(runtime, message, state); | ||
| const generationContext = buildConversationContext(message, state); |
There was a problem hiding this comment.
✅ Correct Usage: The runtime parameter has been correctly removed from the buildConversationContext call. This is the critical fix that enables cloud provider compatibility.
Verification: All 6 actions (createWorkflow, activateWorkflow, deactivateWorkflow, deleteWorkflow, getExecutions) have been updated consistently. Good job!
| test("includes recent messages in context", () => { | ||
| const runtime = createMockRuntime({ agentId: "agent-001" }); | ||
| const message = createMockMessage({ content: { text: "Activate it" } }); | ||
| test('appends current request to recentMessages', () => { |
There was a problem hiding this comment.
Test Coverage - Missing Case: Consider adding a test for when state.values.recentMessages exists but is not a string type. This would verify the type guard on line 5 of context.ts works correctly.
Example:
test('handles non-string recentMessages gracefully', () => {
const message = createMockMessage({ content: { text: 'Hello' } });
const state = createMockState({
values: { recentMessages: { invalid: 'object' } },
});
const result = buildConversationContext(message, state);
expect(result).toBe('Hello'); // Should fall back to just the message
});
| createMockMessage, | ||
| createMockState, | ||
| } from "../helpers/mockRuntime"; | ||
| import { describe, test, expect } from 'bun:test'; |
There was a problem hiding this comment.
Minor - Code Style: Good job removing the unused createMockRuntime import. The test cleanup is thorough and maintains good coverage.
| entityId: "user-001", | ||
| content: { text: `Message ${i}` }, | ||
| })); | ||
| test('preserves recentMessages formatting from provider', () => { |
There was a problem hiding this comment.
✅ Excellent Test: This test is critical for ensuring the fix works correctly. It verifies that:
- Provider-formatted messages are preserved as-is (no re-parsing or reformatting)
- The "Current request" is properly appended
- Different formatting styles from different providers are supported
This is a key requirement for cross-provider compatibility. Great addition!
| const userId = message.entityId; | ||
| const cacheKey = `workflow_draft:${userId}`; | ||
| const generationContext = buildConversationContext(runtime, message, state); | ||
| const generationContext = buildConversationContext(message, state); |
There was a problem hiding this comment.
Good Practice: This is the most critical usage of buildConversationContext since it's used for workflow generation. The fix ensures that the LLM receives the actual conversation context (user's request) instead of a confirmation message like "yes" or "ok".
The PR description correctly identifies this as fixing workflows being generated from confirmation messages on cloud - this change directly addresses that issue.
| { | ||
| "name": "@elizaos/plugin-n8n-workflow", | ||
| "version": "1.0.10", | ||
| "version": "1.0.11", |
There was a problem hiding this comment.
Version Bump: Patch version bump (1.0.10 → 1.0.11) is appropriate for a bug fix. Good adherence to semantic versioning.
Architecture NotesProvider Abstraction Pattern: This PR demonstrates good understanding of the ElizaOS provider pattern:
This is the correct approach for plugin development - plugins should:
Deployment Recommendation:
Version Bump: |
1 participant
Summary
buildConversationContextto usestate.values.recentMessagesinstead ofstate.data.recentMessagesruntimeparameter frombuildConversationContextfunctionProblem
The plugin was failing on cloud because it read from
state.data.recentMessages, which only exists in the official bootstrap provider. The cloud provider uses a different structure (state.data.messages).The correct path is
state.values.recentMessages— a formatted string available on both cloud and official bootstrap providers.Test plan
Summary by CodeRabbit