Skip to content

Add memory provider evaluation harness - #158

Open
yourconscience wants to merge 4 commits into
fix/hermes-memory-syncfrom
feat/memory-provider-eval
Open

Add memory provider evaluation harness#158
yourconscience wants to merge 4 commits into
fix/hermes-memory-syncfrom
feat/memory-provider-eval

Conversation

@yourconscience

@yourconscience yourconscience commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Stacked on #156 (base branch). Implements the evaluation plan from the session-local .hermes/plans/ file.

What

Privacy-safe, fully isolated harness at memory/eval/:

  • CLI (evaluate.py): privacy-scan (regex categories + local canary file; values never printed), capabilities (plugin manifests + runtime listing), run (isolated arms; privacy scan gates execution; --top-k validated 1-50).
  • Adapters: builtin (bounded MEMORY.md emulation, 2200-char budget enforced on ingest/update/capture, query-independent injection) and memsearch (real binary, throwaway dotagents_eval_* collections, strict reset surfaced on failure, teardown resets). Unconfigured providers report capability_gap, never zero scores.
  • Scoring: Recall@1/3/5, MRR, nDCG@5 computed over answerable queries only; abstention accuracy over all queries (unanswerable = correct abstention; answerable = any result counts as non-abstention). Per-query rankings, latencies, lifecycle post-checks, restart/export info under a versioned schema.
  • Fixture (testdata/v1): 12 fully synthetic docs, 18 queries (15 answerable + 3 abstention), 3 mutations, 3 scripted conversations. Codename actors only, .test host, /srv/fixture paths, one inert canary token. privacy_manifest.json records pending_human_review until explicitly approved.
  • Tests: 16 unit tests covering fixture validation (incl. mutation refs), scoring semantics (incl. unanswerable exclusion), adapter budgets/lifecycle, runner gap handling, capability discovery.

Verified end to end (single-run, directional)

Provider Recall@1 MRR Abstention
memsearch (real index+search) 0.933 0.967 0.833
built-in (budget-bound) 0.067 0.119 0.833

Abstention 0.833 = all 15 answerable queries answered, 0/3 unanswerable refused (pure retrieval never abstains — known gap requiring an LLM judge or threshold).

All bot reviews green: DeepSource Python/Go/JS/Secrets/Shell pass, Sourcery pass (blocking security + bug-risk findings addressed: scoring split, builtin update budget, strict reset, top-k validation, centralized subprocess helper). go test ./..., go vet, both Python suites pass locally.

Deliberately out of scope

Cloud/local provider drivers (honcho, mem0, holographic, ...) need configured isolated instances — recorded as capability gaps with explicit reasons, per the plan's rule that gaps are not quality zeros.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@sourcery-ai

sourcery-ai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Reviewer's Guide

Introduces a privacy-safe memory-provider evaluation harness with synthetic fixtures, isolated built-in and memsearch baselines, explicit capability-gap handling, deterministic retrieval metrics, lifecycle checks, and structured versioned results.

Sequence diagram for a privacy-gated evaluation run

sequenceDiagram
    participant CLI as evaluate.py
    participant Scan as Privacy scan
    participant Runner as Evaluation runner
    participant Adapter as Provider adapter
    participant Metrics as Scorer
    participant Output as Results JSON

    CLI->>Scan: scan_fixture()
    alt findings detected
        Scan-->>CLI: finding_count > 0
        CLI-->>Output: abort before ingest
    else fixture is clean
        Scan-->>Runner: fixture hash and clean report
        Runner->>Adapter: reset()
        Runner->>Adapter: ingest(documents)
        loop each query
            Runner->>Adapter: query(query, top_k)
            Adapter-->>Runner: ranked evidence IDs
        end
        loop each mutation
            Runner->>Adapter: update(mutation) or forget(mutation)
            Runner->>Adapter: query(base_query, top_k)
        end
        Runner->>Metrics: score_rankings(queries, rankings)
        Metrics-->>Runner: Recall, MRR, nDCG, abstention
        Runner->>Adapter: export()
        Runner->>Adapter: teardown()
        Runner->>Output: write versioned result payload
    end
Loading

File-Level Changes

Change Details Files
Added a privacy-gated CLI for validating synthetic fixtures, discovering provider capabilities, and running isolated evaluation arms.
  • Implemented deterministic regex and canary scanning without printing sensitive values.
  • Added capability inventory from plugin manifests and Hermes runtime help.
  • Added versioned result output, overwrite controls, provider selection, and explicit failure/capability-gap statuses.
memory/eval/evaluate.py
memory/eval/README.md
docs/memory.md
Implemented isolated baseline adapters with bounded built-in-memory behavior and real memsearch integration.
  • Emulated MEMORY.md under a per-run sandbox with a 2200-character budget and update/forget lifecycle operations.
  • Indexed and queried memsearch using throwaway dotagents_eval_* collections, with reset and teardown cleanup.
  • Represented unavailable providers as capability gaps instead of quality-score zeros.
memory/eval/evaluate.py
Added deterministic evaluation scoring, lifecycle verification, and structured diagnostics.
  • Calculated Recall@1/3/5, MRR, nDCG@5, and abstention accuracy.
  • Recorded per-query rankings, latency summaries, lifecycle post-checks, capture/restart/export results, environment metadata, and fixture hashes.
  • Validated mutation and evidence references before execution.
memory/eval/evaluate.py
memory/eval/README.md
Added a frozen synthetic v1 fixture and automated coverage for privacy, validation, scoring, adapters, runner behavior, and capability discovery.
  • Provided synthetic documents, queries, conversations, mutations, and a pending-human-review privacy manifest.
  • Covered privacy categorization, fixture reference validation, metric determinism, adapter isolation and budgets, lifecycle behavior, capability gaps, and manifest/runtime discovery.
memory/eval/testdata/v1/conversations.jsonl
memory/eval/testdata/v1/documents.jsonl
memory/eval/testdata/v1/mutations.jsonl
memory/eval/testdata/v1/privacy_manifest.json
memory/eval/testdata/v1/queries.jsonl
memory/eval/tests/test_evaluate.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepsource-io

deepsource-io Bot commented Sep 3, 2026

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in 7afec63...89f49a2 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade   Security  

Reliability  

Complexity  

Hygiene  

Code Review Summary

Analyzer Status Updated (UTC) Details
Go Sep 3, 2026 6:47a.m. Review ↗
JavaScript Sep 3, 2026 6:47a.m. Review ↗
Python Sep 3, 2026 6:47a.m. Review ↗
Shell Sep 3, 2026 6:47a.m. Review ↗
Secrets Sep 3, 2026 6:47a.m. Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

Comment thread memory/eval/evaluate.py Outdated
import hashlib
import json
import math
import os

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import os


An object has been imported but is not used anywhere in the file.
It should either be used or the import should be removed.

Comment thread memory/eval/evaluate.py
return records


def load_fixture(root: Path) -> dict[str, list[dict[str, Any]]]:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`load_fixture` has a cyclomatic complexity of 30 with "very-high" risk


A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.

Comment thread memory/eval/evaluate.py Outdated
expected = query.get("expected_evidence_ids", [])
if not isinstance(expected, list):
raise ValueError(f"query {query_id} expected_evidence_ids must be a list")
unknown = sorted(set(str(value) for value in expected) - evidence_ids)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary generator - rewrite as a set comprehension


It is unnecessary to use list, set, dict around a generator expression to get an object of that type since there are comprehensions for these types.

Comment thread memory/eval/evaluate.py
names = sorted(set(PROVIDER_METADATA) | runtime_names | set(manifests))
matrix: list[dict[str, Any]] = []
for name in names:
manifest = manifests.get(name)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incompatible types in assignment (expression has type "Path | None", variable has type "Path")


Incompatible types detected. Please see the issue message for details.

Comment thread memory/eval/evaluate.py
return round(value, 6)


def score_rankings(queries: list[dict[str, Any]], rankings: dict[str, list[str]]) -> dict[str, Any]:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`score_rankings` has a cyclomatic complexity of 16 with "high" risk


A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.

Comment thread memory/eval/evaluate.py Outdated
return {"available": False, "reason": self.reason}

def ingest(self, documents: list[dict[str, Any]]) -> None:
del documents

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary `del` statement in a local scope


Passing a local variable to a del statement results in that variable being removed from the local namespace. When exiting a function all local variables are deleted, so it is unnecessary to explicitly delete variables in such cases.

Comment thread memory/eval/evaluate.py Outdated
raise RuntimeError(self.reason)

def query(self, query: str, top_k: int) -> list[dict[str, Any]]:
del query, top_k

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary `del` statement in a local scope


Passing a local variable to a del statement results in that variable being removed from the local namespace. When exiting a function all local variables are deleted, so it is unnecessary to explicitly delete variables in such cases.

Comment thread memory/eval/evaluate.py Outdated
return self

def __exit__(self, exc_type: Any, exc: Any, traceback: Any) -> None:
del exc_type, exc, traceback

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary `del` statement in a local scope


Passing a local variable to a del statement results in that variable being removed from the local namespace. When exiting a function all local variables are deleted, so it is unnecessary to explicitly delete variables in such cases.

Comment thread memory/eval/evaluate.py
return {"p50_ms": _round(statistics.median(ordered)), "p95_ms": _round(ordered[index])}


def run_adapter(adapter: ProviderAdapter, fixture: dict[str, list[dict[str, Any]]], top_k: int = 5) -> dict[str, Any]:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`run_adapter` has a cyclomatic complexity of 18 with "high" risk


A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.


class MemsearchAdapterTests(unittest.TestCase):
def test_memsearch_adapter_requires_eval_collection_prefix(self):
with tempfile.TemporaryDirectory() as tmp:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider merging collapsible `With` statements`


Nested with statements can be merged into a single compound with statement.

This issue is raised only when there are no other statements between the nested with statements.
Merging collapsible with statements can decrease cognitive complexity, improving code readability.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 4 security issues, and 4 other issues

Security issues:

  • Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
  • Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
  • Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
  • Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="memory/eval/evaluate.py" line_range="217-224" />
<code_context>
+    for query in queries:
+        expected = set(str(value) for value in query.get("expected_evidence_ids", []))
+        ranked = rankings.get(str(query["id"]), [])
+        is_unanswerable = bool(query.get("unanswerable", not expected))
+        if is_unanswerable:
+            value = 1.0 if not ranked else 0.0
+            for k in recall_sums:
+                recall_sums[k] += value
+            reciprocal_ranks.append(value)
+            ndcgs.append(value)
+            abstention.append(value)
+            continue
+        for k in recall_sums:
</code_context>
<issue_to_address>
**issue (bug_risk):** `abstention_accuracy` only receives values for unanswerable queries, so it reports the same accuracy for a provider that abstains on every answerable query and one that answers all answerable queries. The unanswerable branch also adds correct abstentions to Recall, MRR, and nDCG as if they were successful retrievals, making retrieval metrics depend on abstention behavior.

**Triggers:** When the fixture contains both answerable and unanswerable queries, as v1 does.

**Suggested fix:** Compute abstention accuracy across both answerable and unanswerable queries, and exclude unanswerable queries from retrieval metrics rather than treating correct abstention as a retrieved result.
</issue_to_address>

### Comment 2
<location path="memory/eval/evaluate.py" line_range="330-331" />
<code_context>
+        entry = self._find_entry(str(mutation.get("evidence_id", "")))
+        if entry is None:
+            return {"supported": False, "reason": "unknown evidence id"}
+        entry["text"] = str(mutation.get("new_text", "")).strip()
+        self._persist()
+        return {"supported": True, "changed": 1}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** `update` replaces an entry and persists it without enforcing `self.char_limit`, so a mutation with sufficiently long `new_text` produces a `MEMORY.md` larger than the advertised 2200-character bounded-memory budget.

**Triggers:** When an update mutation's replacement text pushes the rendered memory over the configured character limit.

**Suggested fix:** Apply the same budget enforcement used by `ingest` when updating, rejecting or evicting entries before writing the file.
</issue_to_address>

### Comment 3
<location path="memory/eval/evaluate.py" line_range="367-369" />
<code_context>
+        return {"available": self.binary is not None, "binary": self.binary, "collection": self.collection}
+
+    def reset(self) -> None:
+        if self.binary:
+            subprocess.run([self.binary, "reset", "--collection", self.collection, "--yes"], capture_output=True, text=True, timeout=30, check=False)
+        if self.documents_dir.exists():
+            shutil.rmtree(self.documents_dir)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The memsearch reset subprocess uses `check=False` and its return code is ignored, so a failed reset is reported as success and teardown leaves the evaluation collection and its indexed data behind. This contradicts the guarantee that every throwaway collection is reset during teardown and can leak resources or contaminate later runs if collection cleanup fails.

**Triggers:** When the memsearch reset command returns a nonzero status, such as a backend or permission failure.

**Suggested fix:** Check the reset result and surface the failure in the run result while retrying or otherwise explicitly handling teardown cleanup.
</issue_to_address>

### Comment 4
<location path="memory/eval/evaluate.py" line_range="406-433" />
<code_context>
+        if not self.binary:
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The CLI accepts zero and negative `--top-k` values without validation; with memsearch, `len(ranked) >= top_k` is immediately true after the first result for `top_k <= 0`, so a request for zero results returns one result and produces invalid evaluation rankings.

**Triggers:** When a user invokes `run --top-k 0` or with a negative value.

**Suggested fix:** Require `top_k` to be a positive integer in argument parsing or reject it before querying.
</issue_to_address>

### Comment 5
<location path="memory/eval/evaluate.py" line_range="172" />
<code_context>
            result = subprocess.run([hermes, "memory", "--help"], capture_output=True, text=True, timeout=15, check=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

*Source: opengrep*
</issue_to_address>

### Comment 6
<location path="memory/eval/evaluate.py" line_range="368" />
<code_context>
            subprocess.run([self.binary, "reset", "--collection", self.collection, "--yes"], capture_output=True, text=True, timeout=30, check=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

*Source: opengrep*
</issue_to_address>

### Comment 7
<location path="memory/eval/evaluate.py" line_range="384-390" />
<code_context>
        result = subprocess.run(
            [self.binary, "index", str(self.documents_dir), "--collection", self.collection, "--force"],
            capture_output=True,
            text=True,
            timeout=180,
            check=False,
        )
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

*Source: opengrep*
</issue_to_address>

### Comment 8
<location path="memory/eval/evaluate.py" line_range="408-414" />
<code_context>
        result = subprocess.run(
            [self.binary, "search", query, "--top-k", str(top_k), "--collection", self.collection, "--source-prefix", str(self.documents_dir), "--json-output"],
            capture_output=True,
            text=True,
            timeout=60,
            check=False,
        )
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

*Source: opengrep*
</issue_to_address>

Sourcery assessment

Needs a human reviewer. 7 findings to address first, and the harness runs only in an explicitly invoked evaluation path and uses temporary sandboxes plus throwaway memsearch collections, so a defect should not affect live memory. An interrupted run could leave a bounded evaluation collection or result file behind after the code is reverted, but those artifacts can be cleared or regenerated.

Blocking findings: memory/eval/evaluate.py:224, memory/eval/evaluate.py:331, memory/eval/evaluate.py:369, memory/eval/evaluate.py:172, memory/eval/evaluate.py:368, and 2 more


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread memory/eval/evaluate.py Outdated
Comment thread memory/eval/evaluate.py
Comment thread memory/eval/evaluate.py
Comment thread memory/eval/evaluate.py Outdated
Comment thread memory/eval/evaluate.py Outdated
Comment thread memory/eval/evaluate.py Outdated
Comment thread memory/eval/evaluate.py Outdated
Comment thread memory/eval/evaluate.py Outdated
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 95b6ccb6-5296-4c22-b736-6fb06910e803

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant