fix: salvage summary data when Claude returns observation tags instead of summary tags#1604
Closed
octo-patch wants to merge 1 commit intothedotmack:mainfrom
Closed
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/parser.test.ts (1)
57-93: Assertnotesremains null in salvage-path tests.At Line [70]-Line [76] and Line [90]-Line [93], add
expect(result?.notes).toBeNull()to lock the minimal-salvage contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/parser.test.ts` around lines 57 - 93, In the two salvage-path tests ("salvages summary from observation tags when summary tag is absent (issue `#1546`)" and "salvages summary using facts when observation has no narrative (issue `#1546`)"), add an assertion to ensure the minimal-salvage contract by appending expect(result?.notes).toBeNull() to each test after the existing expectations; locate these cases around the parseSummary(...) calls and add the assertion to verify parseSummary returns notes as null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/parser.test.ts`:
- Around line 25-100: Add tests (in this suite or a companion test file) that
assert parseSummary handles three repository-level contracts: 1) uninitialized
memory session ID NULL — feed parseSummary input that simulates missing/empty
session_id and assert the returned object has session_id === null (reference
symbol: session_id, parseSummary); 2) upgrade version-tracking backward
compatibility — supply legacy upgrade/version markers and assert parseSummary
returns a normalized version field or upgrade_version consistent with current
format (reference symbols: version / upgrade_version, parseSummary); 3)
memory-exclusion prompt tag stripping — include privacy tags like <no_memory> or
<skip_memory> inside the input and assert those tags are removed from any
returned text fields and not leaked into the result (reference symbols:
no_memory, skip_memory, parseSummary); implement each as focused it(...) tests
mirroring the existing style and expectations in the parseSummary test suite.
---
Nitpick comments:
In `@tests/parser.test.ts`:
- Around line 57-93: In the two salvage-path tests ("salvages summary from
observation tags when summary tag is absent (issue `#1546`)" and "salvages summary
using facts when observation has no narrative (issue `#1546`)"), add an assertion
to ensure the minimal-salvage contract by appending
expect(result?.notes).toBeNull() to each test after the existing expectations;
locate these cases around the parseSummary(...) calls and add the assertion to
verify parseSummary returns notes as null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27e8dce9-8552-4d88-aad1-ce7efed67bc7
📒 Files selected for processing (2)
src/sdk/parser.tstests/parser.test.ts
Comment on lines
+25
to
+100
| describe('parseSummary', () => { | ||
| it('returns null when no summary or observation tags present', () => { | ||
| const result = parseSummary('some random text'); | ||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it('returns null when skip_summary tag is present', () => { | ||
| const result = parseSummary('<skip_summary reason="no work done" />'); | ||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it('parses a valid summary block', () => { | ||
| const text = ` | ||
| <summary> | ||
| <request>Fix the bug</request> | ||
| <investigated>Found root cause in parser.ts</investigated> | ||
| <learned>The regex was greedy</learned> | ||
| <completed>Fixed the regex</completed> | ||
| <next_steps>Add tests</next_steps> | ||
| <notes>Low risk change</notes> | ||
| </summary> | ||
| `; | ||
| const result = parseSummary(text); | ||
| expect(result).not.toBeNull(); | ||
| expect(result?.request).toBe('Fix the bug'); | ||
| expect(result?.investigated).toBe('Found root cause in parser.ts'); | ||
| expect(result?.learned).toBe('The regex was greedy'); | ||
| expect(result?.completed).toBe('Fixed the regex'); | ||
| expect(result?.next_steps).toBe('Add tests'); | ||
| expect(result?.notes).toBe('Low risk change'); | ||
| }); | ||
|
|
||
| it('salvages summary from observation tags when summary tag is absent (issue #1546)', () => { | ||
| const text = ` | ||
| <observation> | ||
| <type>observation</type> | ||
| <title>Fixed authentication bug in login handler</title> | ||
| <narrative>The root cause was a missing null check in the token validation logic. Added guard clause to prevent NPE.</narrative> | ||
| <facts> | ||
| <fact>Token validation was missing null check</fact> | ||
| <fact>Added guard clause in auth.ts line 42</fact> | ||
| </facts> | ||
| </observation> | ||
| `; | ||
| const result = parseSummary(text); | ||
| expect(result).not.toBeNull(); | ||
| expect(result?.completed).toBe('Fixed authentication bug in login handler'); | ||
| expect(result?.learned).toBe('The root cause was a missing null check in the token validation logic. Added guard clause to prevent NPE.'); | ||
| expect(result?.request).toBeNull(); | ||
| expect(result?.investigated).toBeNull(); | ||
| expect(result?.next_steps).toBeNull(); | ||
| }); | ||
|
|
||
| it('salvages summary using facts when observation has no narrative (issue #1546)', () => { | ||
| const text = ` | ||
| <observation> | ||
| <type>decision</type> | ||
| <title>Chose SQLite over PostgreSQL</title> | ||
| <facts> | ||
| <fact>SQLite has zero configuration</fact> | ||
| <fact>PostgreSQL requires a running server</fact> | ||
| </facts> | ||
| </observation> | ||
| `; | ||
| const result = parseSummary(text); | ||
| expect(result).not.toBeNull(); | ||
| expect(result?.completed).toBe('Chose SQLite over PostgreSQL'); | ||
| expect(result?.learned).toBe('SQLite has zero configuration; PostgreSQL requires a running server'); | ||
| }); | ||
|
|
||
| it('returns null when observation tags present but no useful data', () => { | ||
| const text = '<observation><type>observation</type></observation>'; | ||
| const result = parseSummary(text); | ||
| expect(result).toBeNull(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add the required repository-level test contracts in this suite (or companion suite).
This new test file does not currently validate uninitialized memory session ID NULL behavior, upgrade version-tracking backward compatibility, or memory-exclusion prompt tag stripping.
If you want, I can draft targeted test cases for those three contracts in a follow-up patch.
As per coding guidelines "All test files should validate against expected NULL values for uninitialized memory session IDs", "Test suites should validate backward compatibility for version tracking in upgrade detection", and "Privacy controls for memory exclusion should be validated through user prompt tag stripping tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/parser.test.ts` around lines 25 - 100, Add tests (in this suite or a
companion test file) that assert parseSummary handles three repository-level
contracts: 1) uninitialized memory session ID NULL — feed parseSummary input
that simulates missing/empty session_id and assert the returned object has
session_id === null (reference symbol: session_id, parseSummary); 2) upgrade
version-tracking backward compatibility — supply legacy upgrade/version markers
and assert parseSummary returns a normalized version field or upgrade_version
consistent with current format (reference symbols: version / upgrade_version,
parseSummary); 3) memory-exclusion prompt tag stripping — include privacy tags
like <no_memory> or <skip_memory> inside the input and assert those tags are
removed from any returned text fields and not leaked into the result (reference
symbols: no_memory, skip_memory, parseSummary); implement each as focused
it(...) tests mirroring the existing style and expectations in the parseSummary
test suite.
|
this is essentially breaking claude-mem, rendering it useless! |
3 tasks
alessandropcostabr
pushed a commit
to alessandropcostabr/claude-mem
that referenced
this pull request
Apr 8, 2026
…tmack#1604) Maps title→completed and narrative→learned when LLM responds with <observation> instead of <summary> tags (~72% of cases). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alessandropcostabr
pushed a commit
to alessandropcostabr/claude-mem
that referenced
this pull request
Apr 8, 2026
…tmack#1604) Maps title→completed and narrative→learned when LLM responds with <observation> instead of <summary> tags (~72% of cases). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
|
Closed during the April 2026 backlog cleanup. The underlying bug is now tracked in #1908, which is the single canonical issue being addressed by the maintainer. Thanks for taking the time to report — your symptoms and repro are captured in the consolidated ticket. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1546
Problem
When Claude responds to a summarize prompt with
<observation>tags instead of<summary>tags (occurring ~72% of the time per the issue report),parseSummarylogs a warning and returnsnull, discarding all the data Claude generated.Solution
Instead of immediately returning
nullwhen<summary>is absent but<observation>is present, the parser now attempts to salvage useful data from the first observation block:<title>→completed<narrative>→learned(falls back to joining<fact>elements if narrative is absent)ParsedSummarywith the salvaged fields, leaving unresolvable fields (request,investigated,next_steps) asnullIf neither title nor learned data can be extracted, the function still returns
nullas before.Testing
Added
tests/parser.test.tswith 6 test cases covering:skip_summarytag handlinglearnednullwhen neither summary nor observation tags presentAll 6 tests pass. Pre-existing test suite unaffected.