Conversation
Fixes #28 - Self-Reflection Context Accumulation Issues Changes: - Enhanced _extractJson() with markdown fallback when JSON parsing fails - Added _extractFieldsFromMarkdown() to parse markdown-formatted LLM responses - Added _isValidReflection() for strict JSON structure validation - Added _hasMinimalReflectionData() for lenient markdown validation - Updated getReflectionHistory() to filter out malformed reflections - Strengthened prompt output requirements to emphasize JSON-only output Testing: - Added 21 new tests for extraction methods - Tests cover: valid JSON, embedded JSON, markdown headers/bullets, quoted examples, edge cases, and mixed format scenarios - All 343 tests pass
WalkthroughSwitch reflection extraction to JSON-first parsing with a markdown fallback, add markdown parsing to build structured reflections, and introduce validators to filter reflections missing required or minimal data. Tests for JSON and markdown extraction and validators were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
![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
🧹 Nitpick comments (2)
plugin-nostr/lib/selfReflection.js (1)
1359-1384: Regex pattern may cause ReDoS with crafted input.The
extractListItemsfunction builds a regex from a pattern and applies it to potentially large, untrusted LLM output. The nested quantifiers ([^\\n]+(?:\\n[-*•]\\s*[^\\n]+)*) combined with thegiflags could cause catastrophic backtracking on adversarial input.Consider adding input length limits or using a simpler, iterative parsing approach:
const extractListItems = (pattern) => { const matches = []; + // Limit input to prevent ReDoS on large responses + const limitedText = text.slice(0, 10000); const regex = new RegExp(pattern + '[:\\s]*([^\\n]+(?:\\n[-*•]\\s*[^\\n]+)*)', 'gi'); - const match = text.match(regex); + const match = limitedText.match(regex);plugin-nostr/test/selfReflection.extraction.test.js (1)
207-251: Consider adding test for very short items being filtered.The implementation filters items with
length > 3. A test verifying this behavior would document the intentional filtering of short/noise items.+ it('filters out very short items (length <= 3)', () => { + const text = `Strengths: +- OK +- A valid strength item +- Yes`; + + const result = engine._extractFieldsFromMarkdown(text); + expect(result.strengths).toEqual(['A valid strength item']); + expect(result.strengths).not.toContain('OK'); + expect(result.strengths).not.toContain('Yes'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin-nostr/lib/selfReflection.js(3 hunks)plugin-nostr/test/selfReflection.extraction.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugin-nostr/test/selfReflection.extraction.test.js (1)
plugin-nostr/lib/selfReflection.js (3)
require(1-1)require(2-2)require(3-3)
🔇 Additional comments (11)
plugin-nostr/lib/selfReflection.js (6)
489-492: LGTM! Good validation filtering.The reflection history now properly filters out invalid reflections using
_hasMinimalReflectionData, which aligns with the PR objective to ensure only valid reflections are included in context.
1295-1296: LGTM! Prompt strengthening addresses root cause.The explicit "NO MARKDOWN, NO EXPLANATIONS, NO CODE BLOCKS" instruction directly addresses the issue of LLMs returning non-JSON responses.
1312-1340: LGTM! Solid JSON-first extraction with fallback.The extraction flow correctly:
- Attempts JSON parsing first
- Validates parsed JSON has required fields
- Falls back to markdown extraction when JSON fails or is incomplete
- Returns null only when both paths fail
The debug logging at each step aids troubleshooting.
1395-1403: Good handling of curly/straight quotes for example extraction.The regex correctly handles both straight quotes (
") and Unicode curly quotes (\u201c,\u201d), which is important since LLMs may output either format.
1408-1418: LGTM! Clear strict validation logic.
_isValidReflectionenforces the required contract: strengths, weaknesses, and recommendations must all be arrays. This matches the PR objective for strict JSON validation.
1420-1428: LGTM! Lenient validation for markdown fallback.
_hasMinimalReflectionDataprovides appropriate flexibility for markdown-extracted data by requiring only 2+ populated fields, acknowledging that markdown responses may not have all fields.plugin-nostr/test/selfReflection.extraction.test.js (5)
1-11: LGTM! Clean test setup.Minimal mock runtime is appropriate for unit testing the extraction logic. Using
consoleas the logger is acceptable for tests.
13-60: LGTM! Good coverage of JSON extraction scenarios.Tests cover the happy path, embedded JSON in text, and validation failure for missing required fields. The assertions verify the correct fields are extracted.
62-162: LGTM! Comprehensive markdown fallback tests.Good coverage of:
- Headers with bullet points
- Alternative label formats ("What you're doing well")
- Quoted example extraction
- Improvements and regressions
- Insufficient data returns null
These tests validate the PR's core objective of handling non-JSON LLM responses.
164-205: LGTM! Validation helper tests cover edge cases.Both
_isValidReflectionand_hasMinimalReflectionDataare tested with valid inputs, missing arrays, null, empty objects, and boundary conditions (exactly 2 fields vs. 1 field).
253-314: LGTM! Integration tests verify prioritization logic.Tests correctly verify that:
- Valid JSON is preferred over markdown
- Malformed JSON triggers markdown fallback
- JSON with missing required fields triggers markdown fallback
This matches the intended behavior from the PR objectives.
- Add input length limit (10KB) to prevent ReDoS on large responses - Fix regex pattern to prevent 'your' from matching 'you're/youre' - Add tests for short item filtering (length > 3) - Add test for 'your' vs 'you're' distinction All 345 tests pass.
Addressed CodeRabbit FeedbackFixed all issues from the review: 1.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin-nostr/lib/selfReflection.js (1)
1342-1409: LGTM! Well-implemented markdown extraction with proper safeguards.The markdown extraction logic is solid:
- ReDoS protection via 10,000-char limit (line 1360)
- Comprehensive regex patterns covering various markdown formats and alternative labels
- Extraction of both bullet lists and quoted examples
- Proper handling of fancy quotes in addition to standard quotes
The regex pattern at line 1390 correctly requires the complete "you're" or "youre" string, preventing false matches with "your". The inline comment correctly documents this fix.
Optional: Consider edge case in comma/semicolon splitting.
Lines 1380-1381 split non-bullet content on
,or;, which could incorrectly split items containing these punctuation marks (e.g., "Be concise, clear, and direct" → ["Be concise", "clear", "and direct"]). This is likely an acceptable tradeoff for the fallback extraction, but you may want to add a minimum length check or only split if multiple substantial phrases are detected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin-nostr/lib/selfReflection.js(3 hunks)plugin-nostr/test/selfReflection.extraction.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin-nostr/test/selfReflection.extraction.test.js
🔇 Additional comments (5)
plugin-nostr/lib/selfReflection.js (5)
489-490: LGTM! Improved reflection history filtering.The addition of
_hasMinimalReflectionDatavalidation ensures only reflections with meaningful content are included in the context, preventing accumulation of malformed entries. This directly addresses the context accumulation issues described in the linked issue.
1295-1296: LGTM! Strengthened prompt instructions.The explicit requirement for a single valid JSON object with no markdown, explanations, or code blocks directly addresses the root cause identified in issue #28. This defensive prompt engineering should reduce the frequency of markdown responses.
1317-1340: LGTM! Robust extraction flow with proper fallback.The enhanced extraction logic correctly implements the JSON-first approach with markdown fallback:
- Attempts JSON extraction and validation
- Falls back to markdown parsing on failure
- Uses appropriate validators for each format (_isValidReflection for JSON, _hasMinimalReflectionData for markdown)
- Logs each step for debugging
This addresses the core issue of silent failures when LLM returns non-JSON responses.
1411-1421: LGTM! Appropriate strict validation for JSON reflections.The strict validation requiring
strengths,weaknesses, andrecommendationsarrays ensures JSON-parsed reflections have the complete expected structure. This validator is appropriately strict for JSON input while allowing the more lenient_hasMinimalReflectionDatafor markdown-extracted content.
1423-1431: LGTM! Appropriately lenient validation for markdown extraction.The lenient validation requiring at least 2 populated fields strikes a good balance for markdown-extracted content. This allows reflections with partial data to be preserved while filtering out truly empty or malformed entries. The threshold of 2 fields is reasonable and prevents loss of valuable insights when the LLM returns markdown instead of JSON.
2 participants
Summary
Fixes #28 - Self-Reflection Context Accumulation Issues
Problem
The self-reflection system suffered from context accumulation issues:
Solution
1. Enhanced JSON Extraction with Validation
_extractJson()now validates that parsed JSON has required fields (strengths, weaknesses, recommendations)2. Markdown Fallback Extraction
_extractFieldsFromMarkdown()method parses common markdown patterns:### Strengths:,- item)What you're doing well:,Actionable changes:)Best reply: "text")3. Validation Methods
_isValidReflection(): Strict validation requiring all three arrays_hasMinimalReflectionData(): Lenient validation for markdown (requires 2+ populated fields)getReflectionHistory()now filters using these validators4. Strengthened Prompt
OUTPUT JSON ONLY:to explicit requirements:Testing
selfReflection.extraction.test.jsImpact
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.