test: add unit tests for general_extractor

[mvalentsev]

What does this PR do?

Adds a dedicated test suite for mempalace/general_extractor.py. The module is a pure-heuristic, no-LLM, self-contained extractor that classifies text into decision / preference / milestone / problem / emotional memories, and until now it had no tests of its own. CONTRIBUTING.md explicitly calls out expanding test coverage as a good contribution, so this focuses on locking in the current behaviour with a small, fast test file.

No production code changes.

Coverage added

tests/test_general_extractor.py exercises:

• The happy path for all five memory types (one test each).
• The short-segment filter (segments under 20 chars are dropped).
• The empty-input case.
• min_confidence threshold behaviour (strict vs permissive).
• Fenced code block skipping via _extract_prose (a block full of decision words inside fences does not get classified as a decision).
• Speaker-turn splitting for Human: ... Assistant: ... chat transcripts.
• Sequential chunk_index numbering across multiple segments.
• The shape of the returned dict (keys and types).

How to test

python -m pytest tests/test_general_extractor.py -v
ruff check .
ruff format --check .

12 tests passing locally in ~0.06s.

Checklist

• Tests pass (python -m pytest tests/test_general_extractor.py -v - 12 passed)
• No hardcoded paths
• Linter passes (ruff check ., ruff format --check .)
• Tests run without API keys or network access
• Python 3.9 compatible

[bgauryy]

PR Review: test: add unit tests for general_extractor

Executive Summary

Aspect	Value
PR Goal	Add dedicated test suite for mempalace/general_extractor.py covering all 5 memory types and edge cases
Files Changed	1 (new file)
Risk Level	🟢 LOW - Test-only, no production code changes
Review Effort	2/5 - Small, focused test file
Recommendation	✅ APPROVE (with minor suggestions)

Affected Areas: tests/test_general_extractor.py (new), validates mempalace/general_extractor.py

Business Impact: Improves regression safety for the heuristic memory extractor. Locks in current behavior for all 5 memory types.

Flow Changes: None — no production code modified.

Ratings

Aspect	Score
Correctness	4/5
Security	5/5
Performance	5/5
Maintainability	4/5

PR Health

• Has clear description
• References contribution guidelines (CONTRIBUTING.md)
• Appropriate size (141 lines, single file)
• Has relevant tests (this IS the tests)

Medium Priority Issues

🎨 #1: Test file uses standalone functions instead of class-based grouping

Location: tests/test_general_extractor.py:1-141 | Confidence: ✅ HIGH

Every existing test file in this project groups tests into class Test*: blocks (e.g., TestDialectBasic, TestCompress, TestChunkExchanges). This file uses standalone def test_*() functions, breaking the consistent convention.

- def test_empty_text_returns_empty_list():
-     assert extract_memories("") == []
-
- def test_short_paragraph_is_filtered_out():
-     assert extract_memories("we decided") == []
+ class TestExtractMemoriesBasic:
+     def test_empty_text_returns_empty_list(self):
+         assert extract_memories("") == []
+
+     def test_short_paragraph_is_filtered_out(self):
+         assert extract_memories("we decided") == []

Suggested groupings: TestExtractMemoriesBasic (empty/short), TestMemoryTypes (decision/preference/milestone/problem/emotional), TestEdgeCases (code blocks, speaker turns, confidence), TestOutputFormat (keys, chunk indices).

---

Low Priority Issues

🐛 #2: test_speaker_turns_become_separate_segments doesn't exercise turn-based splitting

Location: tests/test_general_extractor.py:103-118 | Confidence: ✅ HIGH

The test has only 2 speaker turns (Human: + Assistant:), but the production code requires turn_count >= 3 to activate turn-based splitting (line 449 of general_extractor.py). The test falls back to \n\n paragraph splitting. The test name implies turn-splitting is being verified, but it isn't.

To actually test turn-based splitting, add a third turn:

  def test_speaker_turns_become_separate_segments():
      text = (
+         "Human: Let me describe what happened during the deploy.\n\n"
          "Human: We keep running into the same race condition around the "
          "worker startup. The logs make it look like the retry loop is the "
          "root cause of the crash we saw yesterday in staging.\n\n"
          "Assistant: The fix was to add a jittered exponential backoff so "
          "the retries do not pile up on each other. I solved this exact "
          "thing in the previous project and it worked cleanly."
      )

🎨 #3: Misleading comment on fenced-code-block test

Location: tests/test_general_extractor.py:66-67 | Confidence: ⚠️ MED

The comment states "The fenced block is full of decision-ish words but the surrounding prose has none, so nothing should be classified as a decision." In reality, _extract_prose() falls back to the original text when prose extraction yields nothing (line 337). The test passes because the confidence score (0.2) is below the default threshold (0.3), not because the fenced block is excluded from scoring.

The assertion is valid — the end result is correct — but the stated reasoning is misleading. Suggest updating the comment:

- # The fenced block is full of decision-ish words but the surrounding
- # prose has none, so nothing should be classified as a decision.
+ # The fenced block contains decision markers, but the single match
+ # produces a confidence below the default 0.3 threshold.

---

Created by Octocode MCP https://octocode.ai 🔍🐙

[mvalentsev]

Applied the two specific findings:

• test_speaker_turns_become_separate_segments now uses three turns (two Human: plus one Assistant:), which actually exercises _split_by_turns — the switch only kicks in when turn_count >= 3, so the earlier two-turn version was falling back to paragraph splitting.
• Rewrote the comment on test_skips_fenced_code_blocks_when_prose_has_no_markers to describe the real reason the fenced block stays out of the result: _extract_prose strips the block, the surrounding prose has a single decision marker, and the resulting confidence is below the default 0.3 threshold.

Rebased on latest main, 12/12 tests still pass locally. Kept the standalone-function layout since the project already ships both styles (test_config.py, test_miner.py, test_normalize.py are function-based; test_dialect.py, test_knowledge_graph.py are class-based).

[mvalentsev]

Closing - upstream already has comprehensive test coverage for general_extractor (26 tests landed via the recent security-hardening merge). My 12 tests are fully redundant now.