Skip to content

mcp: memory server example #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jul 8, 2025
Merged

Conversation

MegaGrindStone
Copy link
Contributor

Part of #33 . This PR adds an example of implementing go-sdk on memory server.

@jba
Copy link
Contributor

jba commented Jul 7, 2025

I'm not sure I understand the purpose of this example. There is a lot of code implementing an in-memory relational DB, right? But what's the relationship to MCP? How does it illustrate this SDK?

data, err := os.ReadFile(fs.path)
if err != nil {
if os.IsNotExist(err) {
return []byte{}, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

return nil, nil?

return KnowledgeGraph{}, fmt.Errorf("failed to unmarshal from store: %w", err)
}

graph := KnowledgeGraph{
Copy link
Contributor

Choose a reason for hiding this comment

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

KnowledgeGraph{}


for _, obs := range observations {
entityIndex := -1
for i, entity := range graph.Entities {
Copy link
Contributor

Choose a reason for hiding this comment

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

use slices.IndexFunc.

var newEntities []Entity
for _, entity := range entities {
exists := false
for _, existingEntity := range graph.Entities {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use slices.ContainsFunc.


var newObservations []string
for _, content := range obs.Contents {
exists := slices.Contains(graph.Entities[entityIndex].Observations, content)
Copy link
Contributor

Choose a reason for hiding this comment

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

Inline the call into the if statement.


// Filter relations
var filteredRelations []Relation
for _, relation := range graph.Relations {
Copy link
Contributor

Choose a reason for hiding this comment

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

slices.DeleteFunc?


// Filter observations
var filteredObservations []string
for _, observation := range entity.Observations {
Copy link
Contributor

Choose a reason for hiding this comment

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

slices.DeleteFunc?

}

var filteredRelations []Relation
for _, existingRelation := range graph.Relations {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably use one or more slices functions here too.

}

// readGraph returns the complete knowledge graph.
func (k knowledgeBase) readGraph() (KnowledgeGraph, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as loadGraph—why have it?

@jba
Copy link
Contributor

jba commented Jul 7, 2025

Note: I've requested a lot of changes, but they may be moot if we don't have a compelling reason to include this example.

@MegaGrindStone
Copy link
Contributor Author

I'm not sure I understand the purpose of this example. There is a lot of code implementing an in-memory relational DB, right? But what's the relationship to MCP? How does it illustrate this SDK?

This example is listed on the https://modelcontextprotocol.io/examples (mentioned on the first post of #33) as:

Memory - Knowledge graph-based persistent memory system

From the purpose point of view, it might be redundant to other examples, that is, to show how to serve Tools using this sdk.

@MegaGrindStone
Copy link
Contributor Author

Thanks for the feedback, I've addressed it, in case we decide to keep this example in this repo. Also, in this commits, I've already using a new API for this example.

@MegaGrindStone MegaGrindStone requested a review from jba July 8, 2025 09:46
@jba
Copy link
Contributor

jba commented Jul 8, 2025

I see now that it matches an example in the servers repo. I'm OK keeping it. We should find a way to highlight the MCP parts. I'll think about that.

}

// createEntities adds new entities to the graph, skipping duplicates by name.
// Returns the new entities that were actually added.
Copy link
Contributor

Choose a reason for hiding this comment

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

"It returns"


graph, err := k.loadGraph()
if err != nil {
res.IsError = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Just return the error normally, and the SDK will do this for you.


graph, err := k.searchNodes(params.Arguments.Query)
if err != nil {
res.IsError = true
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

// Confirm observation removal
graph, _ = kb.loadGraph()
aliceFound := false
for _, e := range graph.Entities {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you simplify this with a function from the slices package?


observations, err := k.addObservations(params.Arguments.Observations)
if err != nil {
res.IsError = true
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

)

// getStoreFactories provides test factories for both storage implementations.
func getStoreFactories() map[string]func(t *testing.T) store {
Copy link
Contributor

Choose a reason for hiding this comment

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

call this function stores

func TestKnowledgeBaseOperations(t *testing.T) {
factories := getStoreFactories()

for name, factory := range factories {
Copy link
Contributor

Choose a reason for hiding this comment

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

for name, newStore := range stores

same below

}

// Entity represents a knowledge graph node with observations.
type Entity struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the next three should be in kb.go.

@MegaGrindStone MegaGrindStone requested a review from jba July 8, 2025 15:50
jba
jba previously approved these changes Jul 8, 2025
t.Errorf("expected error content in MCP response")
} else {
// Verify the error message contains expected text
expectedErrorMsg := "entity with name NonExistentEntity not found"
Copy link
Contributor

Choose a reason for hiding this comment

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

just want is fine

Copy link
Contributor

@jba jba left a comment

Choose a reason for hiding this comment

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

Thank you!

@jba jba merged commit cdf34cc into modelcontextprotocol:main Jul 8, 2025
3 checks passed
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.

2 participants