feat: optional contradiction detection via Tardygrada

[fabio-rovai]

Summary

• Adds opt-in verify parameter to search_memories() and the mempalace_search MCP tool
• When enabled, pipes retrieved memories through tardygrada verify-doc to detect contradictions before the AI sees them
• Graceful degradation: if tardygrada binary isn't installed, returns results normally with a warning
• Zero impact on default behavior — verify defaults to false

Reopens discussion from #75

How it works

1. After semantic search returns results, writes them to a temp .md file
2. Runs tardygrada verify-doc <tmpfile> (subprocess, 10s timeout)
3. Parses [CONFLICT] blocks from stdout into structured objects
4. Appends contradictions list to the search response

Example

result = search_memories("project status", palace_path, verify=True)
# result["contradictions"] = [
#   {"line_a": 1, "line_b": 3, "claim_a": "project completed on time",
#    "claim_b": "project delayed 3 months", "confidence": 0.85}
# ]

Via MCP:

{"name": "mempalace_search", "arguments": {"query": "project status", "verify": true}}

What this catches

• Temporal contradictions (facts that changed but both versions persist)
• Numeric contradictions ("team has 5 members" vs "8 team members contributed")
• Logical contradictions ("no external APIs used" vs "API costs $2,400")

What doesn't change

• Default behavior (verify=False) — zero impact on existing users
• No new Python dependencies — tardygrada is an optional external binary (314KB, zero deps)
• No changes to storage, indexing, ChromaDB, or knowledge graph
• fact_checker.py untouched (separate concern)

Error handling

• Binary not found → contradictions: null + warning with install link
• Timeout (>10s) → contradictions: null + warning
• Non-zero exit code → contradictions: null + stderr excerpt
• Temp file cleanup guaranteed via finally block

Test plan

• Test: verify detects conflicts (mocked subprocess)
• Test: verify with clean results returns empty list
• Test: missing binary returns null + warning (graceful degradation)
• Test: timeout returns null + warning
• Test: verify=False (default) doesn't call subprocess
• Test: verify=True preserves all search results
• Existing searcher tests pass (7/7 — no regression)
• Existing MCP server tests pass (28/28 — no regression)
• Full test suite passes (107/107)

🤖 Generated with Claude Code

[bgauryy]

PR Review: feat: optional contradiction detection via Tardygrada

Executive Summary

Aspect	Value
PR Goal	Add opt-in verify parameter that pipes search results through tardygrada verify-doc to detect contradictions
Files Changed	3 (+192, -4)
Risk Level	🟡 MEDIUM — introduces subprocess execution with external binary, but opt-in with graceful degradation
Review Effort	3/5 — focused change, but new subprocess pattern requires careful scrutiny
Recommendation	🔄 REQUEST_CHANGES

Affected Areas: mempalace/searcher.py (core verify logic), mempalace/mcp_server.py (MCP wiring), tests/test_verify.py (new test suite)

Business Impact: Enables AI assistants to receive contradiction-flagged search results, reducing hallucination risk from conflicting memories.

Flow Changes: search_memories() gains an optional post-processing step — when verify=True, results are written to a temp file, passed to tardygrada verify-doc, and parsed contradictions are merged into the response dict.

Ratings

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

PR Health

• Has clear description
• References ticket/issue (Integration idea: verify memory consistency with Tardygrada #75)
• Appropriate size
• Has relevant tests — tests exist but ALL will fail (see Congratulations Milla #1)

High Priority Issues

🐛 #1: All 6 tests use non-existent seeded_collection fixture

Location: tests/test_verify.py (all test methods) | Confidence: ✅ HIGH

Every test method takes seeded_collection as a parameter, but this fixture is not defined anywhere in the codebase. tests/conftest.py provides palace_path (empty dir) and palace_with_data (5 pre-loaded drawers). All 6 tests will fail with fixture 'seeded_collection' not found.

The intended fixture is almost certainly palace_with_data, which returns a populated palace_path.

  class TestVerifyWithTardygrada:
-     def test_verify_detects_conflicts(self, palace_path, seeded_collection):
+     def test_verify_detects_conflicts(self, palace_with_data):
          mock_result = MagicMock()
          mock_result.stdout = CONFLICT_OUTPUT
          mock_result.returncode = 0

          with patch("mempalace.searcher.subprocess.run", return_value=mock_result):
-             result = search_memories("authentication", palace_path, verify=True)
+             result = search_memories("authentication", palace_with_data, verify=True)

Apply the same pattern to all 6 test methods: replace (self, palace_path, seeded_collection) with (self, palace_with_data) and pass palace_with_data as the palace path argument.

---

🚨 #2: Unhandled OSError / PermissionError from subprocess.run

Location: mempalace/searcher.py:_verify_with_tardygrada() | Confidence: ✅ HIGH

The function catches FileNotFoundError and subprocess.TimeoutExpired, but subprocess.run() can also raise PermissionError (binary exists but not executable) or other OSError subclasses. An uncaught exception here would propagate up to search_memories() callers and break search entirely when verify=True.

Since this is the first use of subprocess in the codebase, establishing a robust error handling pattern is important.

      except subprocess.TimeoutExpired:
          return {
              "contradictions": None,
              "verify_warning": "tardygrada verify-doc timeout after 10s",
          }
+     except OSError as exc:
+         return {
+             "contradictions": None,
+             "verify_warning": f"tardygrada execution error: {exc}",
+         }
      finally:
          if tmp_path and os.path.exists(tmp_path):
              os.unlink(tmp_path)

---

Medium Priority Issues

🏗️ #3: Subprocess introduces external binary dependency — first in codebase

Location: mempalace/searcher.py:24 | Confidence: ⚠️ MED

The project rules state "No API keys. No network calls." While tardygrada is a local binary (not a network call), this is the first subprocess dependency in the entire codebase. Consider:

1. Adding tardygrada to a documented "optional dependencies" section in README or pyproject.toml extras
2. Adding a log.warning() when tardygrada is not found (currently only returns a dict field — no server-side logging)
3. Explicitly passing shell=False as a defensive security measure, even though list-based invocation defaults to it

      result = subprocess.run(
          ["tardygrada", "verify-doc", tmp_path],
          capture_output=True,
          text=True,
          timeout=10,
+         shell=False,
      )

---

🔄 #4: Asymmetric return shape from search_memories()

Location: mempalace/searcher.py:219-231 | Confidence: ⚠️ MED

When verify=False (default), the response has keys {query, filters, results}. When verify=True, it conditionally gains contradictions and possibly verify_warning via response.update(). This asymmetric shape means callers must use .get() to safely access verification fields.

Consider always including the verification keys when verify=True for a predictable shape:

      if verify:
-         response.update(_verify_with_tardygrada(hits))
+         verification = _verify_with_tardygrada(hits)
+         response["contradictions"] = verification.get("contradictions")
+         response["verify_warning"] = verification.get("verify_warning")

      return response

This way, callers always get both keys when they asked for verification, with None as the default for verify_warning when no warning occurred.

---

Low Priority Issues

🎨 #5: Regex parser is tightly coupled to tardygrada output format

Location: mempalace/searcher.py:_parse_conflicts() | Confidence: ⚠️ MED

The regex in _parse_conflicts is tightly coupled to a specific output format from tardygrada verify-doc. If tardygrada changes its output format (even minor whitespace changes), the parser will silently return an empty list — no errors, no warnings. Consider:

• Logging when stdout is non-empty but no conflicts are parsed (could indicate format change)
• Adding a comment noting the expected tardygrada version/format

---

Flow Impact Analysis

search_memories(verify=False)          search_memories(verify=True)
        │                                       │
   query_palace()                          query_palace()
        │                                       │
   build hits[]                            build hits[]
        │                                       │
   return {query,                          write hits → temp .md file
           filters,                             │
           results}                     subprocess: tardygrada verify-doc
                                                │
                                        parse stdout → conflicts[]
                                                │
                                        cleanup temp file
                                                │
                                        return {query, filters, results,
                                                contradictions, verify_warning?}

Callers affected: tool_search() in mcp_server.py — correctly passes verify through. No other callers of search_memories() are impacted since verify defaults to False.

---

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

[fabio-rovai]

Thanks for the thorough review!

Re #1 (seeded_collection fixture): This is a false positive — seeded_collection is defined at tests/conftest.py:91. All 107 tests pass, including the 6 new ones. The reviewer tool may have been looking at an incomplete index of the conftest.

Re #2, #3, #4 — all addressed in a3ce24c:

• Integrating MemPalace with SoulForge's code intelligence system #2: Added OSError catch (covers PermissionError and other OS-level subprocess failures)
• feat: Hermes memory provider integration #3: Added explicit shell=False for defensive security
• Using AAAK as language for agents #4: Response shape is now predictable — both contradictions and verify_warning keys are always present when verify=True (with None defaults)

Re #5 (regex coupling): Noted and accepted for now — both projects are under my control, so format changes can be coordinated. Happy to add version detection if needed.

Full test suite still 107/107 passing.

[bensig]

Closing — adding Tardygrada as a dependency for optional contradiction detection is more weight than we want right now. If this becomes a priority we'll revisit.