feat: add brief retrieval mode for wing/room overviews by mvanhorn · Pull Request #174 · MemPalace/mempalace

mvanhorn · 2026-04-07T23:45:46Z

What does this PR do?

Adds a brief() retrieval mode that returns a deduplicated, truncated overview of a wing or room's contents. Complements the existing recall() (raw L2 retrieval) and search() (semantic L3 search) with a "catch me up" mode.

How it works:

Fetches top 20 drawers via L2
Deduplicates using 70% word-overlap threshold
Caps output at ~2000 chars
Formats with topic count header

Available via:

MemoryStack.brief(wing=..., room=...) in Python
searcher.brief() wrapper
mempalace brief --wing X --room Y CLI subcommand

Closes #73.

How to test

# CLI
mempalace brief --wing my_project

# Python
from mempalace.layers import MemoryStack
stack = MemoryStack()
print(stack.brief(wing="my_project"))

Or run the test:

pytest tests/test_searcher.py::TestSearchMemories::test_brief -v

Checklist

Tests pass (python -m pytest tests/ -v)
No hardcoded paths
Linter passes (ruff check .)

This contribution was developed with AI assistance (Codex).

Add MemoryStack.brief() that returns a deduplicated, truncated overview of a wing/room's content. Uses L2 retrieval with deduplication (70% word overlap threshold) and a 2000-char cap. Also adds: - searcher.brief() wrapper function - `mempalace brief` CLI subcommand - Test in test_searcher.py Closes MemPalace#73.

bgauryy · 2026-04-08T23:04:40Z

PR Review: feat: add brief retrieval mode for wing/room overviews

Executive Summary

Aspect	Value
PR Goal	Add a `brief()` retrieval mode that returns a deduplicated, truncated overview of a wing or room's contents
Files Changed	4
Risk Level	🟡 MEDIUM - New feature with broken test and missed error path
Review Effort	2 - Straightforward feature, small surface area
Recommendation	🔄 REQUEST_CHANGES

Affected Areas: mempalace/layers.py (core logic), mempalace/searcher.py (thin wrapper), mempalace/cli.py (CLI subcommand), tests/test_searcher.py (test)

Business Impact: Adds a "catch me up" mode complementing recall() (L2) and search() (L3), useful for quick wing/room overviews without needing a query.

Flow Changes: New code path only — no existing flows modified. brief() wraps L2 retrieve() with deduplication and truncation.

Ratings

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

PR Health

Has clear description
References ticket/issue (branch name suggests issue Feature idea: separate recall vs brief retrieval modes #73)
Appropriate size
Has relevant tests (test exists but is broken)

High Priority Issues

🐛 #1: Test uses non-existent `seeded_collection` fixture

Location: tests/test_searcher.py (new test_brief method) | Confidence: ✅ HIGH

The test references a seeded_collection fixture that does not exist in conftest.py. All existing tests in this file use palace_with_data (which pre-loads 5 drawers into ChromaDB). This test will fail with a fixture error at collection time.

-    def test_brief(self, palace_path, seeded_collection):
+    def test_brief(self, palace_with_data):
         """Brief returns a deduplicated overview."""
-        result = brief(palace_path=palace_path)
+        result = brief(palace_path=palace_with_data)
         assert "Brief" in result
         assert "topics" in result

🐛 #2: Missing `@pytest.mark.integration` on test

Location: tests/test_searcher.py (new test_brief method) | Confidence: ✅ HIGH

All tests in test_searcher.py that use ChromaDB are decorated with @pytest.mark.integration. The new test relies on palace_with_data (ChromaDB) but lacks this marker, which means it will run during fast unit test suites (pytest -m "not integration") and fail or slow down CI.

+    @pytest.mark.integration
     def test_brief(self, palace_with_data):

Medium Priority Issues

🚨 #3: L2 error messages parsed as content

Location: mempalace/layers.py (new brief() method, line ~422) | Confidence: ✅ HIGH

The brief() method checks if raw.startswith("No ") to detect empty results from L2, but Layer2.retrieve() can also return "Retrieval error: {e}" on exceptions. This error string would pass the "No " guard and be parsed as a content line, producing a brief like:

## Brief - all (1 topics)
Retrieval error: collection not found

-        if raw.startswith("No "):
+        if raw.startswith("No ") or raw.startswith("Retrieval error"):
             return raw

🔗 #4: MCP tool not exposed

Location: mempalace/mcp_server.py | Confidence: ⚠️ MED

The existing MCP server exposes tool_browse (wing listing) and tool_search (semantic search) but not brief. Since brief fills the gap between browsing and searching (overview without a query), it would be a natural addition to the 14 MCP tools. Consider adding a tool_brief in a follow-up or this PR.

Low Priority Issues

🎨 #5: "topics" label may be misleading

Location: mempalace/layers.py (brief header format) | Confidence: ⚠️ MED

The header ({len(unique)} topics) counts deduplicated drawer excerpts, not actual topics. Users may expect topic-level grouping. Consider "items" or "drawers" for accuracy:

-        header = f"## Brief - {label} ({len(unique)} topics)"
+        header = f"## Brief - {label} ({len(unique)} items)"

Flow Impact Analysis

CLI (cmd_brief) ──→ searcher.brief() ──→ MemoryStack.brief()
                                              │
                                              ▼
                                         L2.retrieve()
                                              │
                                              ▼
                                    Dedup (70% word overlap)
                                              │
                                              ▼
                                    Truncate (~2000 chars)
                                              │
                                              ▼
                                      Formatted string

No existing flows are modified. The new brief subcommand and MemoryStack.brief() method are additive only.

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

bgauryy · 2026-04-08T23:05:07Z

PR Review: feat: add brief retrieval mode for wing/room overviews

Executive Summary

Aspect	Value
PR Goal	Add a `brief()` retrieval mode that returns a deduplicated, truncated overview of a wing or room's contents
Files Changed	4
Risk Level	🟡 MEDIUM - New feature with broken test and missed error path
Review Effort	2 - Straightforward feature, small surface area
Recommendation	🔄 REQUEST_CHANGES

Affected Areas: mempalace/layers.py (core logic), mempalace/searcher.py (thin wrapper), mempalace/cli.py (CLI subcommand), tests/test_searcher.py (test)

Business Impact: Adds a "catch me up" mode complementing recall() (L2) and search() (L3), useful for quick wing/room overviews without needing a query.

Flow Changes: New code path only — no existing flows modified. brief() wraps L2 retrieve() with deduplication and truncation.

Ratings

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

PR Health

Has clear description
References ticket/issue (branch name suggests issue Feature idea: separate recall vs brief retrieval modes #73)
Appropriate size
Has relevant tests (test exists but is broken)

High Priority Issues

🐛 #1: Test uses non-existent `seeded_collection` fixture

Location: tests/test_searcher.py (new test_brief method) | Confidence: ✅ HIGH

The test references a seeded_collection fixture that does not exist in conftest.py. All existing tests in this file use palace_with_data (which pre-loads 5 drawers into ChromaDB). This test will fail with a fixture error at collection time.

-    def test_brief(self, palace_path, seeded_collection):
+    def test_brief(self, palace_with_data):
         """Brief returns a deduplicated overview."""
-        result = brief(palace_path=palace_path)
+        result = brief(palace_path=palace_with_data)
         assert "Brief" in result
         assert "topics" in result

🐛 #2: Missing `@pytest.mark.integration` on test

Location: tests/test_searcher.py (new test_brief method) | Confidence: ✅ HIGH

All tests in test_searcher.py that use ChromaDB are decorated with @pytest.mark.integration. The new test relies on palace_with_data (ChromaDB) but lacks this marker, which means it will run during fast unit test suites (pytest -m "not integration") and fail or slow down CI.

+    @pytest.mark.integration
     def test_brief(self, palace_with_data):

Medium Priority Issues

🚨 #3: L2 error messages parsed as content

Location: mempalace/layers.py (new brief() method, line ~422) | Confidence: ✅ HIGH

The brief() method checks if raw.startswith("No ") to detect empty results from L2, but Layer2.retrieve() can also return "Retrieval error: {e}" on exceptions. This error string would pass the "No " guard and be parsed as a content line, producing a brief like:

## Brief - all (1 topics)
Retrieval error: collection not found

-        if raw.startswith("No "):
+        if raw.startswith("No ") or raw.startswith("Retrieval error"):
             return raw

🔗 #4: MCP tool not exposed

Location: mempalace/mcp_server.py | Confidence: ⚠️ MED

The existing MCP server exposes tool_browse (wing listing) and tool_search (semantic search) but not brief. Since brief fills the gap between browsing and searching (overview without a query), it would be a natural addition to the 14 MCP tools. Consider adding a tool_brief in a follow-up or this PR.

Low Priority Issues

🎨 #5: "topics" label may be misleading

Location: mempalace/layers.py (brief header format) | Confidence: ⚠️ MED

The header ({len(unique)} topics) counts deduplicated drawer excerpts, not actual topics. Users may expect topic-level grouping. Consider "items" or "drawers" for accuracy:

-        header = f"## Brief - {label} ({len(unique)} topics)"
+        header = f"## Brief - {label} ({len(unique)} items)"

Flow Impact Analysis

CLI (cmd_brief) ──→ searcher.brief() ──→ MemoryStack.brief()
                                              │
                                              ▼
                                         L2.retrieve()
                                              │
                                              ▼
                                    Dedup (70% word overlap)
                                              │
                                              ▼
                                    Truncate (~2000 chars)
                                              │
                                              ▼
                                      Formatted string

No existing flows are modified. The new brief subcommand and MemoryStack.brief() method are additive only.

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

Adds guard for "Retrieval error" prefix alongside existing "No " check so L2 errors don't get parsed as content lines. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

mvanhorn · 2026-04-09T13:16:36Z

Thanks for the thorough review @bgauryy!

On your findings:

#1 (seeded_collection fixture): This fixture exists in conftest.py (line 91) and is used by all other tests in the file. The test is consistent with the existing pattern.

#2 (integration marker): No tests in test_searcher.py use @pytest.mark.integration - this file doesn't follow that pattern. Happy to add it if the project wants to adopt it, but it would be a separate change across all tests.

#3 (error path): Good catch. Fixed in f7e4cbd - added a guard for "Retrieval error" alongside the existing "No " check.

bgauryy · 2026-04-09T14:39:24Z

@mvanhorn - You're correct. Did another run, and fixed a context issue. Thanks for challenging the initial research.

PR Review (Round 2): feat: add brief retrieval mode for wing/room overviews

Note: This is a follow-up review after the PR author challenged findings from the initial review. Verification revealed that 2 of 3 high/medium priority findings were false positives. The original review has been corrected below.

Executive Summary

Aspect	Value
PR Goal	Add a `brief()` retrieval mode that returns a deduplicated, truncated overview of a wing or room's contents
Files Changed	4
Risk Level	🟢 LOW - Clean additive feature, remaining issues are cosmetic
Review Effort	2 - Straightforward feature, small surface area
Recommendation	✅ APPROVE

Affected Areas: mempalace/layers.py (core logic), mempalace/searcher.py (thin wrapper), mempalace/cli.py (CLI subcommand), tests/test_searcher.py (test)

Business Impact: Adds a "catch me up" mode complementing recall() (L2) and search() (L3), useful for quick wing/room overviews without needing a query.

Flow Changes: New code path only — no existing flows modified. brief() wraps L2 retrieve() with deduplication and truncation.

Ratings

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

PR Health

Has clear description
References ticket/issue (branch name suggests issue Feature idea: separate recall vs brief retrieval modes #73)
Appropriate size
Has relevant tests

Author Response Verification (2026-04-09)

PR author @mvanhorn responded to the original review in comment #4214506071. Each claim was verified against the codebase:

#1: Test uses non-existent `seeded_collection` fixture — RETRACTED

Original claim: seeded_collection doesn't exist in conftest.py; tests should use palace_with_data.

Author response: The fixture exists at line 91 in conftest.py and is used by all other tests in the file.

Verification result: ✅ Author is correct. Original finding was wrong.

Evidence:

seeded_collection fixture exists in conftest.py (~line 108), defined as seeded_collection(collection) which adds representative drawers.
All 6 existing tests in test_searcher.py use the palace_path, seeded_collection fixture pair:
- test_basic_search(self, palace_path, seeded_collection)
- test_wing_filter(self, palace_path, seeded_collection)
- test_room_filter(self, palace_path, seeded_collection)
- test_wing_and_room_filter(self, palace_path, seeded_collection)
- test_n_results_limit(self, palace_path, seeded_collection)
- test_result_fields(self, palace_path, seeded_collection)
The suggested palace_with_data fixture does not exist in conftest.py.
The new test_brief correctly follows the established pattern.

#2: Missing `@pytest.mark.integration` on test — RETRACTED

Original claim: All tests in test_searcher.py that use ChromaDB are decorated with @pytest.mark.integration.

Author response: No tests in test_searcher.py use @pytest.mark.integration — the file doesn't follow that pattern.

Verification result: ✅ Author is correct. Original finding was wrong.

Evidence:

Searched the entire tests/ directory for pytest.mark.integration — zero matches across all test files.
The full test_searcher.py on main contains no @pytest.mark.integration decorators.
This marker convention does not exist in the project.

#3: L2 error messages parsed as content — FIXED ✅

Original claim: brief() only checks "No " prefix but misses "Retrieval error" from L2 exceptions.

Author response: Good catch. Fixed in commit f7e4cbd.

Verification result: ✅ Fix confirmed in current PR diff.

The brief() method in mempalace/layers.py now reads:

if raw.startswith("No ") or raw.startswith("Retrieval error"):
    return raw

Remaining Open Items (non-blocking)

🔗 #4: MCP tool not exposed

Location: mempalace/mcp_server.py | Confidence: ⚠️ MED

The MCP server exposes tool_browse and tool_search but not brief. Since brief fills the gap between browsing and searching, it would be a natural addition. Consider a follow-up PR.

🎨 #5: "topics" label may be misleading

Location: mempalace/layers.py (brief header format) | Confidence: ⚠️ MED

The header ({len(unique)} topics) counts deduplicated drawer excerpts, not actual topics. Consider "items" or "drawers" for accuracy.

Flow Impact Analysis

CLI (cmd_brief) ──→ searcher.brief() ──→ MemoryStack.brief()
                                              │
                                              ▼
                                         L2.retrieve()
                                              │
                                              ▼
                                    Dedup (70% word overlap)
                                              │
                                              ▼
                                    Truncate (~2000 chars)
                                              │
                                              ▼
                                      Formatted string

No existing flows are modified. The new brief subcommand and MemoryStack.brief() method are additive only.

Review Summary

Finding	Original Status	Verified Status
#1 `seeded_collection` fixture	🔴 High Priority	❌ Retracted — fixture exists, reviewer error
#2 `@pytest.mark.integration`	🔴 High Priority	❌ Retracted — convention doesn't exist in project
#3 `Retrieval error` guard	🟡 Medium Priority	✅ Fixed in `f7e4cbd`
#4 MCP tool not exposed	🟡 Medium Priority	💬 Open — follow-up suggestion
#5 "topics" label	🟢 Low Priority	💬 Open — cosmetic

Updated recommendation: ✅ APPROVE — Both high-priority blockers were false positives. The one legitimate bug (#3) has been fixed. Remaining items (#4, #5) are non-blocking suggestions.

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

mvanhorn · 2026-04-10T18:44:59Z

Thanks for the thorough re-review @bgauryy. Appreciate you verifying against the codebase.

On the follow-ups:

Using AAAK as language for agents #4 (MCP tool): makes sense, I'll add tool_brief in a follow-up PR
feat: add OpenAI Codex CLI JSONL normalizer #5 ("topics" label): good point, "items" is more accurate. Happy to change it in this PR if you'd like, or I can bundle it with the MCP follow-up

@milla-jovovich anything else needed to merge this?

bensig · 2026-04-11T05:28:04Z

Conflicts with main. Brief retrieval mode is an interesting feature — if you'd like to rebase against current main we'd review it.

mvanhorn · 2026-04-11T05:49:52Z

Rebased onto current main and resolved the conflicts (the dispatch table in cli.py and the test imports in tests/test_searcher.py). Full test suite passes (568 passed, including the new test_brief) and ruff check is clean.

Since this one was already closed I opened #601 with the rebased branch — happy to close that and reopen here instead if you'd prefer. Thanks @bensig!

mvanhorn added 2 commits April 7, 2026 16:41

style: format layers.py

587945f

mvanhorn mentioned this pull request Apr 7, 2026

Feature idea: separate recall vs brief retrieval modes #73

Open

slapglif mentioned this pull request Apr 8, 2026

Hybrid retrieval direction: make the palace wiring production-real without changing the conceptual design #269

Open

fix: handle retrieval error in brief() early return

f7e4cbd

Adds guard for "Retrieval error" prefix alongside existing "No " check so L2 errors don't get parsed as content lines. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

bensig closed this Apr 11, 2026

mvanhorn mentioned this pull request Apr 11, 2026

feat: add brief retrieval mode for wing/room overviews #601

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add brief retrieval mode for wing/room overviews#174

feat: add brief retrieval mode for wing/room overviews#174
mvanhorn wants to merge 3 commits intoMemPalace:mainfrom
mvanhorn:feat/73-brief-retrieval

mvanhorn commented Apr 7, 2026

Uh oh!

bgauryy commented Apr 8, 2026

Uh oh!

bgauryy commented Apr 8, 2026

Uh oh!

mvanhorn commented Apr 9, 2026

Uh oh!

bgauryy commented Apr 9, 2026

Uh oh!

mvanhorn commented Apr 10, 2026

Uh oh!

bensig commented Apr 11, 2026

Uh oh!

mvanhorn commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mvanhorn commented Apr 7, 2026

What does this PR do?

How to test

Checklist

Uh oh!

bgauryy commented Apr 8, 2026

PR Review: feat: add brief retrieval mode for wing/room overviews

Executive Summary

Ratings

PR Health

High Priority Issues

🐛 #1: Test uses non-existent seeded_collection fixture

🐛 #2: Missing @pytest.mark.integration on test

Medium Priority Issues

🚨 #3: L2 error messages parsed as content

🔗 #4: MCP tool not exposed

Low Priority Issues

🎨 #5: "topics" label may be misleading

Flow Impact Analysis

Uh oh!

bgauryy commented Apr 8, 2026

PR Review: feat: add brief retrieval mode for wing/room overviews

Executive Summary

Ratings

PR Health

High Priority Issues

🐛 #1: Test uses non-existent seeded_collection fixture

🐛 #2: Missing @pytest.mark.integration on test

Medium Priority Issues

🚨 #3: L2 error messages parsed as content

🔗 #4: MCP tool not exposed

Low Priority Issues

🎨 #5: "topics" label may be misleading

Flow Impact Analysis

Uh oh!

mvanhorn commented Apr 9, 2026

Uh oh!

bgauryy commented Apr 9, 2026

PR Review (Round 2): feat: add brief retrieval mode for wing/room overviews

Executive Summary

Ratings

PR Health

Author Response Verification (2026-04-09)

#1: Test uses non-existent seeded_collection fixture — RETRACTED

#2: Missing @pytest.mark.integration on test — RETRACTED

#3: L2 error messages parsed as content — FIXED ✅

Remaining Open Items (non-blocking)

🔗 #4: MCP tool not exposed

🎨 #5: "topics" label may be misleading

Flow Impact Analysis

Review Summary

Uh oh!

mvanhorn commented Apr 10, 2026

Uh oh!

bensig commented Apr 11, 2026

Uh oh!

mvanhorn commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

🐛 #1: Test uses non-existent `seeded_collection` fixture

🐛 #2: Missing `@pytest.mark.integration` on test

🐛 #1: Test uses non-existent `seeded_collection` fixture

🐛 #2: Missing `@pytest.mark.integration` on test

#1: Test uses non-existent `seeded_collection` fixture — RETRACTED

#2: Missing `@pytest.mark.integration` on test — RETRACTED