Skip to content

fix(memory): route memory provider tools in sequential dispatch path - #4709

Closed
Hygaard wants to merge 2 commits into
NousResearch:mainfrom
Hygaard:fix/memory-provider-sequential-dispatch
Closed

fix(memory): route memory provider tools in sequential dispatch path#4709
Hygaard wants to merge 2 commits into
NousResearch:mainfrom
Hygaard:fix/memory-provider-sequential-dispatch

Conversation

@Hygaard

@Hygaard Hygaard commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds memory provider tool routing to the sequential dispatch path in _execute_tool_calls_sequential, which was missing the memory_manager.has_tool() check that already exists in the concurrent path (_invoke_tool).

Fixes #4708

Problem

After setting up the holographic memory provider plugin, calling fact_store or fact_feedback returns {"error": "Unknown tool: fact_store"}.

The root cause: _should_parallelize_tool_batch() returns False for single tool calls (len <= 1), so they always go through the sequential path. The sequential path's elif chain handles todo, session_search, memory, clarify, and delegate_task explicitly, then falls through to handle_function_call()registry.dispatch() for everything else. Since memory provider tools are registered in MemoryManager._tool_to_provider (not the main tool registry), they get "Unknown tool."

The concurrent path (_invoke_tool) already handles this correctly at line ~5620:

elif self._memory_manager and self._memory_manager.has_tool(function_name):
    return self._memory_manager.handle_tool_call(function_name, function_args)

Changes

1. Add memory_manager routing to sequential path (run_agent.py) — inserted between the delegate_task handler and the handle_function_call fallthrough:

elif self._memory_manager and self._memory_manager.has_tool(function_name):
    function_result = self._memory_manager.handle_tool_call(function_name, function_args)
    ...

2. Add on_memory_write bridge to sequential memory handler (run_agent.py) — the concurrent path already mirrors built-in memory writes to the external provider via on_memory_write(), but the sequential path was missing this. Added the same bridge after the memory_tool() call.

3. Regression tests (tests/test_run_agent.py) — three new tests in TestMemoryProviderSequentialDispatch:

  • test_sequential_routes_memory_provider_tool — single fact_store call routes through memory_manager
  • test_sequential_memory_write_bridge — built-in memory writes trigger on_memory_write on the provider
  • test_sequential_no_memory_manager_falls_through — no provider configured → normal fallthrough

All 224 existing tests pass alongside the 3 new ones.

Note

Filed as issue #4708 first — we're not 100% certain this is a bug vs. a misunderstanding of the architecture. If the sequential path intentionally doesn't route memory provider tools, please let us know and we'll close both.

Bug found by Claude Opus 4.6 running through Hermes Agent during a debugging session.

The sequential tool dispatch path (_execute_tool_calls_sequential) was
missing the memory_manager.has_tool() check that exists in the concurrent
path (_invoke_tool). Since single tool calls always use the sequential
path (len <= 1), memory provider tools like fact_store and fact_feedback
would fall through to registry.dispatch() which returns 'Unknown tool'.

Changes:
- Add memory_manager.has_tool() routing to the sequential elif chain,
  between delegate_task and the handle_function_call fallthrough
- Add on_memory_write bridge to the sequential memory handler, matching
  the concurrent path behavior

Fixes NousResearch#4708
…ousResearch#4708)

Three tests covering the memory provider tool routing fix:

1. test_sequential_routes_memory_provider_tool — verifies that a single
   fact_store call routes through memory_manager.handle_tool_call()
   instead of falling through to registry.dispatch()

2. test_sequential_memory_write_bridge — verifies that built-in memory
   writes in the sequential path trigger on_memory_write() on the
   external memory provider

3. test_sequential_no_memory_manager_falls_through — verifies that when
   no memory provider is configured (memory_manager is None), tools
   fall through to handle_function_call() as before
@Hygaard

Hygaard commented Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #4803 which shipped in v0.7.0. Same fix, same root cause. Closing to keep things tidy.

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.

Possible bug: memory provider tools (fact_store, fact_feedback) may not route correctly in sequential dispatch path

1 participant