Fix duplicate replies to home feed posts #72
Conversation
|
Warning Rate limit exceeded@anabelle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 32 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded in-memory deduplication checks to skip posting duplicate replies in both home-feed and discovery reply paths; also added tests verifying home-feed deduplication across repeated processing runs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Scheduler as processHomeFeed
participant Service as NostrService
participant Memory as Runtime Memory
participant LLM as LLM / Text Gen
participant Images as Image Processor
participant Post as postReply
Note over Scheduler,Post: Normal flow (no prior reply)
Scheduler->>Service: process event
Service->>Service: compute context/thread
Service->>LLM: generate reply text
LLM-->>Service: text
Service->>Memory: createUniqueUuid(evt.id) & getMemories(...)
Memory-->>Service: return recent memories (no match)
Service->>Images: optional image processing
Images-->>Service: images processed
Service->>Post: postReply(text)
Post-->>Service: posted ✓
Note over Scheduler,Post: Dedupe flow (prior reply exists)
Scheduler->>Service: process same event
Service->>Service: compute context/thread
Service->>LLM: generate reply text
LLM-->>Service: text
Service->>Memory: createUniqueUuid(evt.id) & getMemories(...)
Memory-->>Service: return recent memories (match found)
Service->>Service: log skip, do not call Images or Post
Service-->>Scheduler: skip posting ✗
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
Adds reply deduplication to home feed processing to prevent multiple replies to the same post and introduces tests verifying the behavior.
- Implements a memory check before posting a reply in processHomeFeed
- Adds unit tests covering reply when no prior reply exists and skip when a prior reply exists
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| plugin-nostr/lib/service.js | Adds deduplication check in home feed reply path before calling postReply |
| plugin-nostr/test/service.homeFeedDeduplication.test.js | Introduces tests to validate deduplication behavior for home feed replies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Add reply deduplication logic in processHomeFeed function to prevent the agent from replying multiple times to the same home feed post. The fix checks for existing replies in memory before posting a new reply, similar to the deduplication logic used for mention handling. This resolves issue #71: Bug: Duplicate replies to home feed posts - Added deduplication check before calling postReply in home feed reply logic - Check looks for existing replies in recent memory (up to 100 messages) - Uses the same pattern as mention handling for consistency
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugin-nostr/test/service.homeFeedDeduplication.test.js (2)
80-167: Refactor duplicated mock setup into shared helper.Lines 85–104 and 140–159 repeat nearly identical mock assignments. Extract this common setup into a helper function to reduce duplication and improve maintainability.
Example refactor:
function setupHomeFeedMocks(service) { service._loadCurrentContacts = vi.fn(() => Promise.resolve(new Set(['test-pubkey']))); service._list = vi.fn(() => Promise.resolve([{ id: 'test-event-id', pubkey: 'test-pubkey', content: 'Test post content', created_at: Math.floor(Date.now() / 1000), kind: 1 }])); service._isQualityContent = vi.fn(() => true); service._analyzePostForInteraction = vi.fn(() => Promise.resolve(true)); service._isUserMuted = vi.fn(() => Promise.resolve(false)); service._getThreadContext = vi.fn(() => Promise.resolve([])); service._getConversationIdFromEvent = vi.fn(() => 'test-conv-id'); service._ensureNostrContext = vi.fn(() => Promise.resolve({ roomId: 'test-room-id' })); service._shouldEngageWithThread = vi.fn(() => true); service.generateReplyTextLLM = vi.fn(() => Promise.resolve('Test reply text')); service.postReply = vi.fn(() => Promise.resolve(true)); service.imageProcessingEnabled = false; service.createUniqueUuid = vi.fn((_, seed) => `uuid-${seed}`); service._chooseInteractionType = vi.fn(() => 'reply'); }Then call
setupHomeFeedMocks(service);in each test.
81-81: Remove console.log debugging statements.Console.log statements at lines 81, 122, and 124 are debugging artifacts and should be removed before merging.
Apply this diff:
- console.log('Starting test: should not reply twice'); service = await NostrService.start(mockRuntime); // ... - // First call to processHomeFeed - should reply - console.log('Calling processHomeFeed first time'); + // First call to processHomeFeed - should reply await service.processHomeFeed(); - console.log('processHomeFeed completed first time, postReply calls:', service.postReply.mock.calls.length); expect(service.postReply).toHaveBeenCalledTimes(1);Also applies to: 122-122, 124-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin-nostr/lib/service.js(1 hunks)plugin-nostr/test/service.homeFeedDeduplication.test.js(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test plugin-nostr on PR
plugin-nostr/test/service.homeFeedDeduplication.test.js
[error] 125-125: AssertionError: expected postReply to have been called 1 times, but was called 0 times in 'should not reply twice to the same home feed post'.
[error] 166-166: AssertionError: expected postReply to have been called 1 times, but was called 0 times in 'should reply when no existing reply is found'.
🪛 Gitleaks (8.28.0)
plugin-nostr/test/service.homeFeedDeduplication.test.js
[high] 16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
plugin-nostr/lib/service.js (1)
5856-5893: Run dedup before invoking the LLMThe new dedup guard still calls
generateReplyTextLLMon every pass before discovering that we already replied. In practice, this means the second (and third, …) cron runs will continue to hit the expensive model call on the same event even though we skip posting, defeating a big part of the “reduce unnecessary API calls/processing” goal. Please perform the memory lookup and early-return before the LLM invocation so repeated runs bail out without spending tokens.plugin-nostr/test/service.homeFeedDeduplication.test.js (1)
16-16: Static analysis false positive: hardcoded key is a test fixture.The static analysis tool flags line 16 as containing a sensitive API key. This is a test fixture (dummy private key for mocking), not a real secret. You can safely ignore this warning or add a gitleaks exception comment if needed.
anabelle
force-pushed
the
fix-duplicate-home-feed-replies
branch
from
b2fca2c to
83f9792
Compare
- Move deduplication check before LLM call to prevent unnecessary API usage - Clean up test code by removing console.log statements and refactoring duplicated mocks - Add helper function for test setup to improve maintainability
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| useModel: vi.fn(() => Promise.resolve({ text: 'Test reply' })), | ||
| createMemory: vi.fn(), | ||
| getMemoryById: vi.fn(), | ||
| getMemories: vi.fn(() => []), |
There was a problem hiding this comment.
[nitpick] Mock getMemories to return a promise consistently (it’s awaited in production code) to avoid accidental sync/async mismatches: getMemories: vi.fn(() => Promise.resolve([])).
| getMemories: vi.fn(() => []), | |
| getMemories: vi.fn(() => Promise.resolve([])), |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugin-nostr/test/service.homeFeedDeduplication.test.js (1)
110-134: Critical: Closure scope prevents deduplication test from exercising the dedupe path.The
getMemoriesmock (lines 110-122) capturescallCountin its closure. ResettingcallCount = 0on line 129 doesn't affect the closure's captured variable—the secondprocessHomeFeed()call continues incrementing from the closure's state, likely returning[]instead of the expected reply array. This prevents the deduplication logic from being tested.Apply this diff to fix the mock sequencing:
- // Mock getMemories to return existing reply on second call - let callCount = 0; - mockRuntime.getMemories = vi.fn(() => { - callCount++; - if (callCount === 1) { - // First call - no existing replies - return Promise.resolve([]); - } else { - // Second call - existing reply found - return Promise.resolve([{ - content: { inReplyTo: 'uuid-test-event-id' } - }]); - } - }); + // Mock getMemories to return no existing replies on first call + mockRuntime.getMemories = vi.fn() + .mockResolvedValueOnce([]) // First call - no existing replies + .mockResolvedValue([{ // Subsequent calls - existing reply found + content: { inReplyTo: 'uuid-test-event-id' } + }]); // First call to processHomeFeed - should reply await service.processHomeFeed(); expect(service.postReply).toHaveBeenCalledTimes(1); - // Reset call count and mocks for second call - callCount = 0; service.postReply.mockClear(); // Second call to processHomeFeed - should NOT reply due to deduplication await service.processHomeFeed(); expect(service.postReply).not.toHaveBeenCalled();Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin-nostr/lib/service.js(1 hunks)plugin-nostr/test/service.homeFeedDeduplication.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugin-nostr/lib/service.js (3)
plugin-nostr/test-eliza-integration.js (1)
recent(49-49)plugin-nostr/lib/context.js (3)
require(175-175)roomId(6-6)roomId(151-151)plugin-nostr/lib/lnpixels-listener.js (7)
require(1-1)require(13-13)require(14-14)require(88-88)require(89-89)roomId(17-17)roomId(91-91)
🪛 GitHub Actions: Test plugin-nostr on PR
plugin-nostr/test/service.homeFeedDeduplication.test.js
[error] 1-1: Command: npm run test. Failure in Home Feed Deduplication tests: expected spy to be called 1 times, but got 0 times (should not reply twice to the same home feed post).
[error] 1-1: Command: npm run test. Failure in Home Feed Deduplication tests: expected spy to be called 1 times, but got 0 times (should reply when no existing reply is found).
🪛 Gitleaks (8.28.0)
plugin-nostr/test/service.homeFeedDeduplication.test.js
[high] 16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
plugin-nostr/test/service.homeFeedDeduplication.test.js (1)
16-16: Gitleaks warning is a false positive—test fixture properly documented.The static analysis tool flags this as a potential secret exposure, but the inline comment clearly identifies it as a test fixture. No action needed unless your team prefers to add a Gitleaks suppression comment to silence the warning in CI.
| const shouldEngage = this._shouldEngageWithThread(evt, threadContext); | ||
| if (!shouldEngage) { | ||
| logger.debug(`[NOSTR] Home feed skipping reply to ${evt.id.slice(0, 8)} after thread analysis - not suitable for engagement`); | ||
| success = false; | ||
| break; | ||
| } | ||
|
|
||
| // Check if we've already replied to this event (early exit to avoid unnecessary LLM calls) | ||
| const eventMemoryId = this.createUniqueUuid(this.runtime, evt.id); | ||
| const recent = await this.runtime.getMemories({ tableName: 'messages', roomId, count: 100 }); | ||
| const hasReply = recent.some((m) => m.content?.inReplyTo === eventMemoryId || m.content?.inReplyTo === evt.id); | ||
| if (hasReply) { | ||
| logger.info(`[NOSTR] Skipping home feed reply to ${evt.id.slice(0, 8)} (found existing reply)`); | ||
| success = false; | ||
| break; | ||
| } | ||
|
|
||
| // Process images in home feed post content (if enabled) | ||
| let imageContext = { imageDescriptions: [], imageUrls: [] }; | ||
| if (this.imageProcessingEnabled) { | ||
| try { | ||
| logger.info(`[NOSTR] Processing images in home feed post: "${evt.content?.slice(0, 200)}..."`); | ||
| const { processImageContent } = require('./image-vision'); | ||
| const fullImageContext = await processImageContent(evt.content || '', this.runtime); | ||
| imageContext = { | ||
| imageDescriptions: fullImageContext.imageDescriptions.slice(0, this.maxImagesPerMessage), | ||
| imageUrls: fullImageContext.imageUrls.slice(0, this.maxImagesPerMessage) | ||
| }; | ||
| logger.info(`[NOSTR] Processed ${imageContext.imageDescriptions.length} images from home feed post`); | ||
| } catch (error) { | ||
| logger.error(`[NOSTR] Error in home feed image processing: ${error.message || error}`); | ||
| imageContext = { imageDescriptions: [], imageUrls: [] }; | ||
| } | ||
| } | ||
|
|
||
| const text = await this.generateReplyTextLLM(evt, roomId, threadContext, imageContext); | ||
|
|
||
| // Check if LLM generation failed (returned null) | ||
| if (!text || !text.trim()) { | ||
| logger.warn(`[NOSTR] Skipping home feed reply to ${evt.id.slice(0, 8)} - LLM generation failed`); | ||
| success = false; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| const text = await this.generateReplyTextLLM(evt, roomId, threadContext, imageContext); | ||
|
|
||
| // Check if LLM generation failed (returned null) | ||
| if (!text || !text.trim()) { | ||
| logger.warn(`[NOSTR] Skipping home feed reply to ${evt.id.slice(0, 8)} - LLM generation failed`); | ||
| success = false; | ||
| break; | ||
| } | ||
|
|
||
| success = await this.postReply(evt, text); | ||
|
|
||
| success = await this.postReply(evt, text); | ||
| break; |
There was a problem hiding this comment.
Missing reply memory makes dedupe ineffective
We now check getMemories() before replying, but nothing in this path ever writes a memory whose content.inReplyTo matches the home‑feed event. After a restart (or whenever homeFeedProcessedEvents is cleared) that query still comes back empty, so the agent will reply again—the original bug remains. Persist the reply the way mention handling already does so the dedupe gate has something to find.
- const { roomId } = await this._ensureNostrContext(evt.pubkey, undefined, convId);
+ const { roomId, entityId } = await this._ensureNostrContext(evt.pubkey, undefined, convId);
@@
- success = await this.postReply(evt, text);
+ success = await this.postReply(evt, text);
+ if (success) {
+ try {
+ const replyMemoryId = this.createUniqueUuid(this.runtime, `${evt.id}:reply:${Date.now()}`);
+ await this._createMemorySafe({
+ id: replyMemoryId,
+ entityId,
+ agentId: this.runtime.agentId,
+ roomId,
+ content: {
+ text,
+ source: 'nostr',
+ inReplyTo: eventMemoryId,
+ },
+ createdAt: Date.now(),
+ }, 'messages');
+ } catch (err) {
+ this.logger?.debug?.('[NOSTR] Failed to persist home feed reply memory:', err?.message || err);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
|
@copilot tests are failing |
* Fix duplicate replies to home feed posts Add reply deduplication logic in processHomeFeed function to prevent the agent from replying multiple times to the same home feed post. The fix checks for existing replies in memory before posting a new reply, similar to the deduplication logic used for mention handling. This resolves issue #71: Bug: Duplicate replies to home feed posts - Added deduplication check before calling postReply in home feed reply logic - Check looks for existing replies in recent memory (up to 100 messages) - Uses the same pattern as mention handling for consistency * Address PR review feedback - Move deduplication check before LLM call to prevent unnecessary API usage - Clean up test code by removing console.log statements and refactoring duplicated mocks - Add helper function for test setup to improve maintainability * Fix test setup to properly initialize service pool for processHomeFeed
Description
This PR fixes the issue where the agent was replying multiple times to the same home feed posts, particularly posts with images. The second reply would come approximately 15 minutes after the first reply with a similar but different message.
Root Cause
The
processHomeFeedfunction inplugin-nostr/lib/service.jslacked reply deduplication logic. Unlike mention handling which checks for existing replies before posting, home feed replies could be sent multiple times to the same post.Solution
Added reply deduplication check in the home feed reply logic, similar to the existing deduplication used for mention handling:
Changes
postReplycall in home feed processingTesting
Related Issue
Fixes #71: Bug: Duplicate replies to home feed posts
Impact
Summary by CodeRabbit
Bug Fixes
Tests