Skip to content

fix: only mirror builtin memory writes to external providers after success#667

Open
badMade wants to merge 5 commits into
mainfrom
badmade/fix-mirrored-failed-memory-writes
Open

fix: only mirror builtin memory writes to external providers after success#667
badMade wants to merge 5 commits into
mainfrom
badmade/fix-mirrored-failed-memory-writes

Conversation

@badMade
Copy link
Copy Markdown
Owner

@badMade badMade commented May 31, 2026

Motivation

  • Prevent external memory providers from being poisoned by writes the builtin memory store rejected (e.g. scanner-blocked, invalid targets, duplicates, or quota failures) which could later be injected back into model context.

Description

  • Add a helper AIAgent._memory_tool_write_succeeded(function_result: str) -> bool that parses the memory_tool JSON result and returns True only when success is true.\
  • Gate the external-memory bridge in both invocation paths so MemoryManager.on_memory_write(...) is called only when the builtin write succeeded: the concurrent helper (_invoke_tool) and the sequential path (_execute_tool_calls_sequential).\
  • Preserve existing metadata behaviour and backward compatibility for providers that accept legacy (3-arg) or metadata-aware signatures.\
  • Add regression tests to tests/agent/test_memory_provider.py covering: rejected sequential writes are not mirrored, accepted sequential writes are mirrored, and rejected concurrent writes are not mirrored.

Testing

  • Ran the targeted bridge regression tests: python -m pytest -o addopts='' tests/agent/test_memory_provider.py::TestOnMemoryWriteBridge -q — all tests passed.\
  • Ran the full file tests: PYTHONPATH=/workspace/hermes-agent/.venv/lib/python3.14/site-packages python -m pytest -o addopts='' tests/agent/test_memory_provider.py -q — all tests passed (67 passed).\
  • Verified Python source compiles: python -m py_compile run_agent.py tests/agent/test_memory_provider.py — success.\
  • Notes: running python -m pytest tests/agent/test_memory_provider.py -q in the base interpreter failed due to repository addopts expecting xdist; scripts/run_tests.sh attempted to install a helper package but PyPI access failed in this environment, so tests were executed using the repository venv instead.

Codex Task

Copilot AI review requested due to automatic review settings May 31, 2026 22:31
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request ensures that external memory providers are only notified of memory writes after the built-in memory store has successfully accepted the write. It introduces a helper method _memory_tool_write_succeeded to verify the success of the write operation and updates both sequential and concurrent tool execution paths, supported by new unit tests. Feedback suggests making the helper method more robust by checking if the input is already a dictionary before attempting to parse it as JSON, which prevents potential errors in testing or mocking environments.

Comment thread run_agent.py Outdated
Comment on lines +4357 to +4364
@staticmethod
def _memory_tool_write_succeeded(function_result: str) -> bool:
"""Return True only when the builtin memory tool accepted a write."""
try:
result = json.loads(function_result)
except (TypeError, json.JSONDecodeError):
return False
return isinstance(result, dict) and result.get("success") is True
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

In testing or mocking environments, function_result might be passed as a pre-parsed dictionary instead of a raw JSON string. To make this helper more robust and prevent unexpected TypeError exceptions during test execution, we can check if function_result is already a dictionary before attempting to parse it with json.loads.

    @staticmethod
    def _memory_tool_write_succeeded(function_result: Any) -> bool:
        """Return True only when the builtin memory tool accepted a write."""
        if isinstance(function_result, dict):
            return function_result.get("success") is True
        try:
            result = json.loads(function_result)
        except (TypeError, json.JSONDecodeError):
            return False
        return isinstance(result, dict) and result.get("success") is True

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@gemini-code-assist, is this suggestion still valid? If so, what are the steps and line numbers to make this fix?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, the suggestion remains valid to ensure robustness in testing environments where the function_result might already be a dictionary. You can apply this change to run_agent.py by replacing the existing _memory_tool_write_succeeded method (lines 4358-4364) with the following implementation:

Suggested change
@staticmethod
def _memory_tool_write_succeeded(function_result: str) -> bool:
"""Return True only when the builtin memory tool accepted a write."""
try:
result = json.loads(function_result)
except (TypeError, json.JSONDecodeError):
return False
return isinstance(result, dict) and result.get("success") is True
@staticmethod
def _memory_tool_write_succeeded(function_result: Any) -> bool:
"""Return True only when the builtin memory tool accepted a write."""
if isinstance(function_result, dict):
return function_result.get("success") is True
try:
result = json.loads(function_result)
except (TypeError, json.JSONDecodeError):
return False
return isinstance(result, dict) and result.get("success") is True

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 31, 2026

🔎 Lint report: badmade/fix-mirrored-failed-memory-writes vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 8249 on HEAD, 8249 on base (➖ 0)

🆕 New issues (22):

Rule Count
invalid-argument-type 16
unsupported-operator 3
unresolved-attribute 3
First entries
run_agent.py:13602: [invalid-argument-type] invalid-argument-type: Argument to function `len` is incorrect: Expected `Sized`, found `(str & ~AlwaysFalsy) | (dict[Unknown, Unknown] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:3397: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_stale_timeout` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
tests/run_agent/test_provider_attribution_headers.py:156: [unsupported-operator] unsupported-operator: Operator `not in` is not supported between objects of type `Literal["X-OpenRouter-Cache-TTL"]` and `Unknown | str | dict[str, str] | ... omitted 3 union elements`
run_agent.py:2633: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `(Unknown & ~AlwaysFalsy) | (str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | ... omitted 4 union elements`
run_agent.py:5441: [invalid-argument-type] invalid-argument-type: Argument to function `parse_rate_limit_headers` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:13099: [invalid-argument-type] invalid-argument-type: Argument to function `estimate_usage_cost` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:13599: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 3 union elements`
tests/agent/test_codex_cloudflare_headers.py:163: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str & ~AlwaysFalsy`, `int & ~AlwaysFalsy` in union `(Unknown & ~AlwaysFalsy) | (str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:2377: [invalid-argument-type] invalid-argument-type: Argument to function `query_ollama_num_ctx` is incorrect: Expected `str`, found `(str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 4 union elements`
run_agent.py:9132: [invalid-argument-type] invalid-argument-type: Argument to function `get_transport` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:2630: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `(Unknown & ~AlwaysFalsy) | (str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | ... omitted 4 union elements`
tests/agent/test_codex_cloudflare_headers.py:181: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["originator"]` and `(Unknown & ~AlwaysFalsy) | (str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:13136: [invalid-argument-type] invalid-argument-type: Argument to bound method `SessionDB.update_token_counts` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:7335: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 3 union elements`
run_agent.py:9570: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_profile` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
tests/run_agent/test_provider_attribution_headers.py:90: [unresolved-attribute] unresolved-attribute: Attribute `startswith` is not defined on `dict[str, str]` in union `Unknown | str | dict[str, str]`
run_agent.py:13833: [invalid-argument-type] invalid-argument-type: Argument to function `_pool_may_recover_from_rate_limit` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:2690: [invalid-argument-type] invalid-argument-type: Argument to function `get_model_context_length` is incorrect: Expected `str`, found `str | dict[str, str] | Any | ... omitted 3 union elements`
run_agent.py:7164: [invalid-argument-type] invalid-argument-type: Argument to function `_codex_cloudflare_headers` is incorrect: Expected `str`, found `Unknown | str | dict[str, str] | ... omitted 3 union elements`
tests/agent/test_codex_cloudflare_headers.py:163: [unresolved-attribute] unresolved-attribute: Attribute `startswith` is not defined on `dict[str, str]` in union `Unknown | str | dict[str, str]`
tests/run_agent/test_provider_attribution_headers.py:155: [unsupported-operator] unsupported-operator: Operator `not in` is not supported between objects of type `Literal["X-OpenRouter-Cache"]` and `Unknown | str | dict[str, str] | ... omitted 3 union elements`
run_agent.py:13052: [invalid-argument-type] invalid-argument-type: Argument to function `normalize_usage` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`

✅ Fixed issues (27):

Rule Count
invalid-argument-type 19
unresolved-attribute 5
unsupported-operator 3
First entries
run_agent.py:2377: [invalid-argument-type] invalid-argument-type: Argument to function `query_ollama_num_ctx` is incorrect: Expected `str`, found `(str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 5 union elements`
run_agent.py:9114: [invalid-argument-type] invalid-argument-type: Argument to function `get_transport` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
run_agent.py:2633: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `(Unknown & ~AlwaysFalsy) | (str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | ... omitted 5 union elements`
run_agent.py:7763: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_request_timeout` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
run_agent.py:9979: [unresolved-attribute] unresolved-attribute: Attribute `lower` is not defined on `dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy`, `int & ~AlwaysFalsy`, `dict[Unknown, Unknown] & ~AlwaysFalsy` in union `(str & ~AlwaysFalsy) | (Unknown & ~AlwaysFalsy) | (dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy) | ... omitted 4 union elements`
run_agent.py:5423: [invalid-argument-type] invalid-argument-type: Argument to function `parse_rate_limit_headers` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
run_agent.py:13807: [invalid-argument-type] invalid-argument-type: Argument to function `_pool_may_recover_from_rate_limit` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
run_agent.py:9552: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_profile` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
run_agent.py:2690: [invalid-argument-type] invalid-argument-type: Argument to function `get_model_context_length` is incorrect: Expected `str`, found `str | dict[str, str] | Any | ... omitted 4 union elements`
run_agent.py:13026: [invalid-argument-type] invalid-argument-type: Argument to function `normalize_usage` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
run_agent.py:13110: [invalid-argument-type] invalid-argument-type: Argument to bound method `SessionDB.update_token_counts` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
run_agent.py:4285: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
run_agent.py:14030: [invalid-argument-type] invalid-argument-type: Argument to bound method `ContextCompressor.update_model` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
tests/run_agent/test_provider_attribution_headers.py:156: [unsupported-operator] unsupported-operator: Operator `not in` is not supported between objects of type `Literal["X-OpenRouter-Cache-TTL"]` and `Unknown | str | dict[str, str] | ... omitted 4 union elements`
tests/agent/test_codex_cloudflare_headers.py:163: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str & ~AlwaysFalsy`, `int & ~AlwaysFalsy` in union `(Unknown & ~AlwaysFalsy) | (str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | ... omitted 4 union elements`
run_agent.py:13576: [invalid-argument-type] invalid-argument-type: Argument to function `len` is incorrect: Expected `Sized`, found `(str & ~AlwaysFalsy) | (dict[Unknown, Unknown] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 4 union elements`
run_agent.py:7146: [invalid-argument-type] invalid-argument-type: Argument to function `_codex_cloudflare_headers` is incorrect: Expected `str`, found `Unknown | str | dict[str, str] | ... omitted 4 union elements`
run_agent.py:13073: [invalid-argument-type] invalid-argument-type: Argument to function `estimate_usage_cost` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
run_agent.py:3397: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_stale_timeout` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | ... omitted 3 union elements`
tests/agent/test_codex_cloudflare_headers.py:181: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["originator"]` and `(Unknown & ~AlwaysFalsy) | (str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | ... omitted 4 union elements`
run_agent.py:7317: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 4 union elements`
run_agent.py:2630: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `(Unknown & ~AlwaysFalsy) | (str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | ... omitted 5 union elements`
run_agent.py:11556: [unresolved-attribute] unresolved-attribute: Attribute `strip` is not defined on `dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy`, `int & ~AlwaysFalsy`, `dict[Unknown, Unknown] & ~AlwaysFalsy` in union `(str & ~AlwaysFalsy) | (Unknown & ~AlwaysFalsy) | (dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy) | ... omitted 4 union elements`
run_agent.py:13573: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 4 union elements`
tests/run_agent/test_provider_attribution_headers.py:155: [unsupported-operator] unsupported-operator: Operator `not in` is not supported between objects of type `Literal["X-OpenRouter-Cache"]` and `Unknown | str | dict[str, str] | ... omitted 4 union elements`
... and 2 more

Unchanged: 4333 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to prevent “poisoning” external memory providers by ensuring the external-memory bridge is only invoked after the built-in memory tool has actually accepted a write.

Changes:

  • Add AIAgent._memory_tool_write_succeeded(function_result: str) -> bool to interpret the memory tool’s JSON result.
  • Gate the external memory bridge in both the concurrent (_invoke_tool) and sequential (_execute_tool_calls_sequential) tool execution paths based on write success.
  • Add regression tests in tests/agent/test_memory_provider.py to verify rejected writes are not mirrored and successful writes are mirrored.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
run_agent.py Adds a helper and uses it to gate external memory mirroring after built-in memory writes.
tests/agent/test_memory_provider.py Adds tests covering sequential/concurrent bridging behavior for accepted vs rejected built-in writes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread run_agent.py Outdated
Comment on lines +4360 to +4364
try:
result = json.loads(function_result)
except (TypeError, json.JSONDecodeError):
return False
return isinstance(result, dict) and result.get("success") is True
Comment on lines +1059 to +1076
agent._execute_tool_calls_sequential(
SimpleNamespace(
tool_calls=[
_memory_tool_call({
"action": "add",
"target": "memory",
"content": accepted_content,
})
]
),
messages,
"task-1",
)

result = json.loads(messages[0]["content"])
assert result["success"] is True
assert agent._memory_store.memory_entries == [accepted_content]
assert external.memory_writes == [("add", "memory", accepted_content)]
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 31, 2026

Auto-merge: review received — action required

Review activity detected (4 inline comment(s) by non-author(s); 2 comment(s)/review(s) from review bot(s): gemini-code-assist[bot], copilot-pull-request-reviewer[bot]).

To confirm you have reviewed and accepted the feedback, please add the reviewed label to this PR. Auto-merge will then proceed.

View workflow run

@badMade
Copy link
Copy Markdown
Owner Author

badMade commented May 31, 2026

@claude code review

@badMade
Copy link
Copy Markdown
Owner Author

badMade commented May 31, 2026

@jules code review

@badMade
Copy link
Copy Markdown
Owner Author

badMade commented Jun 1, 2026

@copilot, fix all failing checks.

- _memory_tool_write_succeeded() now returns False when the builtin
  store responds with success=true but message contains "already exists"
  (i.e. duplicate-entry no-op), preventing those from being mirrored to
  external providers.
- Add regression test: test_sequential_memory_bridge_skips_duplicate_builtin_write
  verifies that a second identical add is not forwarded to the external provider.
Copy link
Copy Markdown

Copilot AI commented Jun 1, 2026

@copilot, fix all failing checks.

Fixed in the latest commit. _memory_tool_write_succeeded() now also returns False when the builtin store responds with success=true but the message contains "already exists" (i.e. the no-op duplicate case), so those writes are no longer mirrored to external providers. A regression test test_sequential_memory_bridge_skips_duplicate_builtin_write was added to cover this case.

The other CI failures (44 gateway tests with NameError: name 'team_id' is not defined and a webhook test assertion) are pre-existing issues unrelated to these changes.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 1, 2026

Auto-merge: checks failing

The following checks did not pass:

  • test (failure)

Please fix the failing checks before this PR can be merged.

View workflow run

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants