Bug: Quote block parsing breaks on empty continuation lines

[malibio]

Closes #855

[malibio]

Code Review: PR #858 - Bug: Quote block parsing breaks on empty continuation lines

Review Type: Initial Review

---

Requirements Check (Issue #855)

Criterion	Status	Notes
Multi-line quotes with empty continuation lines (>) are parsed as single quote-block	✅	Fix applied to both prepare_nodes_from_markdown and parse_markdown
Quote block content preserves the > prefixes correctly (for rendering)	✅	Content is preserved as-is with > prefixes
Add test case for multi-paragraph quotes	✅	3 comprehensive test cases added
Re-import affected documents to fix existing data (or provide migration)	❌	Not addressed in this PR - see note below

Note on migration criterion: The fourth acceptance criterion mentions re-importing affected documents. This is a data-level fix that would need to be done manually or via a separate migration script. The code fix itself is complete. This should either be:

1. Documented as a manual step for affected users
2. Handled in a follow-up issue if systematic migration is needed

---

Code Review Findings

✅ No Critical Issues

The fix is straightforward, correct, and well-tested.

---

🟡 Important: Missing Test Coverage for parse_markdown Path

File: /packages/core/src/mcp/handlers/markdown_test.rs

The fix was applied to two functions:

1. prepare_nodes_from_markdown (batch import) - tested
2. parse_markdown (update operations via handle_update_root_from_markdown) - not tested

While the code changes are identical and the fix is correct, having test coverage for both code paths provides defense against future regressions if these functions diverge.

Recommendation: Consider adding one test using handle_update_root_from_markdown with the quote block pattern, e.g.:

#[tokio::test]
async fn test_update_root_with_quote_block_empty_continuation() {
    let (node_service, _temp_dir) = setup_test_service().await;
    
    // Create root with simple content
    let create_params = json!({
        "markdown_content": "Simple text",
        "sync_import": true,
        "title": "Test"
    });
    let create_result = handle_create_nodes_from_markdown(&node_service, create_params)
        .await.unwrap();
    let root_id = create_result["root_id"].as_str().unwrap();
    
    // Update with multi-line quote containing empty continuation
    let update_params = json!({
        "root_id": root_id,
        "markdown": "> Line 1\n>\n> Line 2"
    });
    
    let result = handle_update_root_from_markdown(&node_service, update_params)
        .await.unwrap();
    
    assert_eq!(result["nodes_created"], 1, "Should create single quote-block");
}

Severity: Low - the implementation is correct, this is a testing completeness suggestion.

---

🟢 Suggestions (Nitpicks)

Nit: /packages/core/src/mcp/handlers/markdown_test.rs:1918-1921

The assertion quote_node.content.contains(">") is a weak test since > would also match the other lines. Consider checking for the specific empty continuation pattern or verifying line count:

// Current - would pass even if empty line was missing
assert!(quote_node.content.contains(">"), "Missing empty continuation line");

// More precise alternative
let lines: Vec<&str> = quote_node.content.lines().collect();
assert_eq!(lines.len(), 4, "Should have 4 lines including empty continuation");
assert_eq!(lines[1], ">", "Second line should be empty continuation");

---

Summary

Overall Assessment: This is a clean, well-implemented bug fix that:

1. Correctly identifies the root cause: The parser only matched "> " (with space), missing empty continuation lines that are just ">".

2. Applies the fix consistently: Both prepare_nodes_from_markdown and parse_markdown are updated with identical logic.

3. Includes comprehensive tests: Three test cases covering:

   • The exact bug scenario from the issue
   • Edge case: quote starting with empty line
   • Production example from real document

4. Maintains backward compatibility: Existing valid quote blocks continue to parse correctly.

5. Good commit message: Clear explanation of the problem, solution, and test coverage.

---

Recommendation: APPROVE

The code change is correct, well-tested, and directly addresses the bug. The suggestions above are minor improvements that could be addressed in this PR or as follow-up work.

---

Reviewed by: Claude Opus 4.5 (Principal Engineer Reviewer)

[malibio]

Review completed. See detailed review comment above. Recommendation: APPROVE - the fix is correct, well-implemented, and adequately tested.

[malibio]

Review Feedback Addressed ✅

All reviewer recommendations have been implemented:

✅ Addressed (2/2)

1. 🟡 Important: Missing test coverage for parse_markdown path

   • Added test_update_root_with_quote_block_empty_continuation test
   • Tests the handle_update_root_from_markdown code path with the issue Bug: Quote block parsing breaks on empty continuation lines #855 pattern
   • Verifies single quote-block creation and content preservation

2. 🟢 Suggestion: Weak assertion precision

   • Replaced weak contains(">") assertion with precise line count and pattern checks
   • Now verifies: 4 lines total, and lines[1] == ">" for empty continuation

⏭️ Skipped: None

All recommendations were reasonable and valuable improvements.

Summary

• Commits created: 1
• Tests: 19/19 quote block tests passing ✅
• Quality checks: All passing ✅

---

Addressed by: Claude Opus 4.5