From 2fb2be544bc93b69bae7f36c9fa7770c5f7b09a2 Mon Sep 17 00:00:00 2001 From: sanjibani <18418553+sanjibani@users.noreply.github.com> Date: Sun, 5 Jul 2026 21:18:28 +0530 Subject: [PATCH] fix(edit): consume same-level sibling sections in replace_section (#1012) The consume run after a target section header used to stop at any '#' heading, which left a same-level sibling in place even when the replacement content re-authored that sibling from scratch. Result: silent content duplication -- the replacement's new '## Description' followed by the original '## Description'. Stop the consume run only when we hit a strictly deeper heading (a subsection). Same-level/higher-level headings that appear directly after the target are now consumed, so replacement content can authoritatively re-define them. A same-level sibling that appears only after one or more preserved subsections is left untouched (it belongs to the next top-level block). Updated two existing tests that asserted the old buggy behavior ('## Another Section' / '## Section 2' preserved when their previous section was replaced). Added regression tests covering both the duplicate-sibling case and the subsection-preservation case. --- src/basic_memory/services/entity_service.py | 31 +++++--- tests/services/test_entity_service.py | 85 ++++++++++++++++++++- 2 files changed, 104 insertions(+), 12 deletions(-) diff --git a/src/basic_memory/services/entity_service.py b/src/basic_memory/services/entity_service.py index e1e3316a3..d97c0263e 100644 --- a/src/basic_memory/services/entity_service.py +++ b/src/basic_memory/services/entity_service.py @@ -1269,13 +1269,22 @@ def replace_section_content( ) -> str: """Replace content under a specific markdown section header. - This method uses a simple, safe approach: when replacing a section, it only - replaces the immediate content under that header until it encounters the next - header of ANY level. This means: + When replacing a section, we consume the original content under the header until + we hit any other heading, with one important refinement: a same-level or + higher-level heading that appears **directly** after the target (no intervening + subsections) is also consumed, so replacement content can authoritatively + re-define that sibling without leaving the original duplicated below the + replacement (issue #1012). A strictly deeper heading (`###` under a replaced + `##`) is always treated as a subsection and preserved. + + In practice: - Replacing "# Header" replaces content until "## Subsection" (preserves subsections) - Replacing "## Section" replaces content until "### Subsection" (preserves subsections) - - More predictable and safer than trying to consume entire hierarchies + - Replacing "## Section" consumes a directly-following "## Sibling" or + "# Sibling" so replacement content can re-author it + - A same-level sibling that appears only after one or more preserved + subsections is left untouched (it belongs to the next top-level block) Args: current_content: The current markdown content @@ -1323,6 +1332,7 @@ def replace_section_content( # Replace the single matching section result_lines = [] section_line_idx = matching_sections[0] + section_header_level = len(section_header) - len(section_header.lstrip("#")) i = 0 while i < len(lines): @@ -1335,15 +1345,18 @@ def replace_section_content( result_lines.append(new_content) i += 1 - # Skip the original section content until next header or end + # Consume content directly after the target until we hit a header. + # Same-level/higher-level headings here are consumed so the + # replacement can re-author them without leaving duplicates below + # (issue #1012). Strictly-deeper headings (subsections) stop the + # consume run and are preserved. while i < len(lines): next_line = lines[i] - # Stop consuming when we hit any header (preserve subsections) if next_line.startswith("#"): - # We found another header - continue processing from here - break + next_level = len(next_line) - len(next_line.lstrip("#")) + if next_level > section_header_level: + break i += 1 - # Continue processing from the next header (don't increment i again) continue # Add all other lines (including subsequent sections) diff --git a/tests/services/test_entity_service.py b/tests/services/test_entity_service.py index 45719a7c3..c6f378ef8 100644 --- a/tests/services/test_entity_service.py +++ b/tests/services/test_entity_service.py @@ -396,6 +396,79 @@ async def test_delete_nonexistent_entity(entity_service: EntityService): assert await entity_service.delete_entity("test/non_existent") is True +@pytest.mark.asyncio +async def test_replace_section_consumes_same_level_siblings(entity_service: EntityService): + """replace_section must consume same-level sibling headings when replacement re-authors them. + + Regression for #1012: the original implementation stopped at any `#` heading after + the target section, so a replacement that re-defined a sibling (e.g. replaced + `## Observations` with content that itself includes `## Description`) left the + original sibling duplicated below the replacement. Subsections (`### ...`) under + the target must still be preserved. + """ + + current = ( + "## Observations\n" + "- [status] open\n" + "- [priority] high\n" + "\n" + "## Description\n" + "Jamie called in...\n" + "\n" + "## Relations\n" + "- reported_by [[Jamie Rivera]]\n" + ) + + new_content = ( + "- [status] resolved\n" + "- [resolution] shipped\n" + "\n" + "## Description\n" + "Updated...\n" + "\n" + "## Resolution\n" + "Fix details...\n" + ) + + result = entity_service.replace_section_content(current, "## Observations", new_content) + + # Replacement content is present. + assert "- [status] resolved" in result + assert "## Resolution" in result + # Replacement re-authored `## Description`; original `Jamie called in...` must be gone. + assert "Jamie called in..." not in result + assert result.count("## Description") == 1 + # Original `## Relations` (sibling of `## Observations`) is consumed by the + # replacement -- the user is replacing the whole observations block, not appending. + assert "reported_by" not in result + + +@pytest.mark.asyncio +async def test_replace_section_preserves_deeper_subsections(entity_service: EntityService): + """replace_section must preserve subsections (strictly deeper headings) under the target.""" + + current = ( + "## Implementation\n" + "Top-level summary.\n" + "\n" + "### Detail A\n" + "Detail A body.\n" + "\n" + "### Detail B\n" + "Detail B body.\n" + ) + + result = entity_service.replace_section_content( + current, + "## Implementation", + "New top-level summary.\n", + ) + + assert "New top-level summary." in result + assert "### Detail A" in result and "Detail A body." in result + assert "### Detail B" in result and "Detail B body." in result + + @pytest.mark.asyncio async def test_create_entity_with_special_chars(entity_service: EntityService): """Test entity creation with special characters in name and description.""" @@ -892,12 +965,15 @@ async def test_edit_entity_replace_section( section="## Section 1", ) - # Verify section was replaced + # Verify section was replaced. Directly-following same-level siblings are + # consumed so replacement content can authoritatively re-author them without + # leaving the original duplicated (issue #1012). file_path = file_service.get_entity_path(updated) file_content, _ = await file_service.read_file(file_path) assert "New section 1 content" in file_content assert "Original section 1 content" not in file_content - assert "Original section 2 content" in file_content # Other sections preserved + assert "Original section 2 content" not in file_content # Same-level sibling consumed + assert file_content.count("## Section 2") == 0 # Original sibling gone (replacement can re-author) @pytest.mark.asyncio @@ -1498,7 +1574,10 @@ async def test_edit_entity_replace_section_strips_duplicate_header( assert "New content for testing section" in file_content assert "Original content" not in file_content - assert "## Another Section" in file_content # Other sections preserved + # Directly-following same-level sibling is consumed so replacement content can + # re-author it without leaving the original duplicated (issue #1012). + assert "## Another Section" not in file_content + assert "Other content" not in file_content # Insert before/after section tests