Skip to content

fix: align query_entity default direction with MCP documentation#611

Closed
arnoldwender wants to merge 1 commit intoMemPalace:developfrom
arnoldwender:fix/kg-query-direction-default
Closed

fix: align query_entity default direction with MCP documentation#611
arnoldwender wants to merge 1 commit intoMemPalace:developfrom
arnoldwender:fix/kg-query-direction-default

Conversation

@arnoldwender
Copy link
Copy Markdown
Contributor

@arnoldwender arnoldwender commented Apr 11, 2026

Problem

API contract mismatch between the KG class and MCP tool description:

  • knowledge_graph.py:196 defaults to direction="outgoing"
  • mcp_server.py:635 documents "default: both" in the MCP tool schema

Users calling mempalace_kg_query via MCP without specifying direction get outgoing-only results, but the tool description promises both directions.

Fix

Change the default in query_entity() from "outgoing" to "both" to match the documented MCP behavior.

Test plan

  • pytest tests/test_knowledge_graph.py tests/test_knowledge_graph_extra.py -v — all 28 tests pass
  • ruff check + ruff format clean
  • Query an entity via MCP without direction param — should return both incoming and outgoing relationships

web3guru888
web3guru888 approved these changes Apr 11, 2026
View reviewed changes
Copy link
Copy Markdown

@web3guru888 web3guru888 left a comment

Choose a reason for hiding this comment

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

API contract mismatches between the Python layer and the MCP schema are the kind of bug that's easy to miss and painful to debug — the caller does exactly what the docs say, gets wrong results, and has no visibility into why. Aligning the default resolves this cleanly.

One thing worth confirming before merge: are there existing callers anywhere in cli.py, mcp_server.py, or tests that rely on the current "outgoing" default behavior? The test plan covers the KG test files, but a quick grep for query_entity( without an explicit direction= argument would rule out any silent behavioral regressions. If all existing call sites pass direction explicitly, this is a no-risk change.

LGTM pending that grep.

[MemPalace-AGI integration — 215 tests, 710 KG entities]

@mvalentsev
Copy link
Copy Markdown
Contributor

mvalentsev commented Apr 11, 2026

#356 makes the same one-line change to query_entity() in knowledge_graph.py (direction="outgoing" -> "both"). #356 is older (Apr 9) and also adds a dedicated regression test for the default behaviour in test_knowledge_graph.py. Worth coordinating before a double-merge.

@bensig bensig changed the base branch from main to develop April 11, 2026 22:21
This was referenced Apr 12, 2026
Closed
Closed
@arnoldwender
Copy link
Copy Markdown
Contributor Author

arnoldwender commented Apr 13, 2026

Ran the grep — confirmed safe to merge.

All real call sites pass direction explicitly:

  • mcp_server.py:495: _kg.query_entity(entity, as_of=as_of, direction=direction)

The only naked query_entity() calls without direction= are in the module docstring (lines 26, 29 of knowledge_graph.py), not in runtime code.

No existing callers rely on the "outgoing" default, so changing it to "both" is a no-regression change.

@arnoldwender
Copy link
Copy Markdown
Contributor Author

arnoldwender commented Apr 13, 2026

Thanks for flagging — #356 was closed without merge, so there's no duplication risk. This PR is the only active one making this change.

As confirmed in my earlier comment, all runtime call sites pass direction explicitly, so this is a zero-regression change.

@igorls igorls added area/kg Knowledge graph area/mcp MCP server and tools bug Something isn't working labels Apr 14, 2026
@arnoldwender
Copy link
Copy Markdown
Contributor Author

arnoldwender commented Apr 14, 2026

Confirmed the fix is already merged in develop. Closing — thanks!

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

Reviewers

@milla-jovovich milla-jovovich Awaiting requested review from milla-jovovich milla-jovovich is a code owner

@bensig bensig Awaiting requested review from bensig bensig is a code owner

1 more reviewer

@web3guru888 web3guru888 web3guru888 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/kg Knowledge graph area/mcp MCP server and tools bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@arnoldwender @mvalentsev @web3guru888 @igorls