Skip to content

fix(mcp): 9 quality improvements for search, save, and pipeline#43

Open
kckylechen1 wants to merge 1 commit intomainfrom
feat/mcp-quality-improvements-v2
Open

fix(mcp): 9 quality improvements for search, save, and pipeline#43
kckylechen1 wants to merge 1 commit intomainfrom
feat/mcp-quality-improvements-v2

Conversation

@kckylechen1
Copy link
Copy Markdown
Owner

Summary

Reapplies fixes from #42 onto the restructured codebase (db split into modules, hub_ops split into subdirectory, tool_params became a directory).

Of the original 13 fixes, 4 were already applied on main (auto_link scope, enrichment concurrency, search skip response, generate_summary fallback). The remaining 9 are applied here.

Changes

  • importance clamp: save_memory now clamps importance to [0.0, 1.0]
  • EXTRACTION_PROMPT + fact_to_entry: Prompt requests entities/persons; fact_to_entry parses them from LLM output
  • cross-DB score normalization: Global and project results re-normalized by各自 max before merge sort
  • hub_feedback false success: hub_record_feedback returns bool; handler returns recorded: false for non-existent IDs; rating clamped [0.0, 5.0]
  • strip_code_fence: Uses rfind("```") for last closing fence (handles nested)
  • noise filter: Added 6 AI boilerplate patterns (I apologize, As an AI, etc.)
  • SKIP_PATTERNS: Long-text check uses named variable references instead of [3..5] slice
  • sync_memories: Reads known state from both global and project DBs

Test plan

  • cargo check — zero warnings
  • cargo test -p memory-core — 39 tests pass

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 introduces several enhancements to the memory system, including updating the feedback mechanism to return a boolean status, clamping rating and importance values, and improving the robustness of code fence stripping in LLM responses. It also extends the memory extraction prompt and logic to support entities and persons, and updates the synchronization process to merge known revisions from project-specific databases. Additionally, a cross-DB score normalization step was added to the search logic to ensure results from global and project scopes are comparable. A review comment suggests optimizing the normalization logic by leveraging the existing sort order to find maximum scores more efficiently.

Comment on lines +251 to +260
let global_max = combined_results
.iter()
.filter(|(_, s)| matches!(s, DbScope::Global))
.map(|(r, _)| r.score.final_score)
.fold(0.0_f64, f64::max);
let project_max = combined_results
.iter()
.filter(|(_, s)| matches!(s, DbScope::Project))
.map(|(r, _)| r.score.final_score)
.fold(0.0_f64, f64::max);
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

Since combined_results is already sorted by final_score in descending order (lines 241-246), calculating global_max and project_max using fold and f64::max is inefficient as it iterates through the entire list. You can simply find the first occurrence for each scope to get the maximum score.

Suggested change
let global_max = combined_results
.iter()
.filter(|(_, s)| matches!(s, DbScope::Global))
.map(|(r, _)| r.score.final_score)
.fold(0.0_f64, f64::max);
let project_max = combined_results
.iter()
.filter(|(_, s)| matches!(s, DbScope::Project))
.map(|(r, _)| r.score.final_score)
.fold(0.0_f64, f64::max);
let global_max = combined_results
.iter()
.find(|(_, s)| matches!(s, DbScope::Global))
.map(|(r, _)| r.score.final_score)
.unwrap_or(0.0);
let project_max = combined_results
.iter()
.find(|(_, s)| matches!(s, DbScope::Project))
.map(|(r, _)| r.score.final_score)
.unwrap_or(0.0);

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.

Removed the per-store re-normalization block instead of micro-optimizing it. That keeps the merged ranking on the raw final scores and avoids flattening relevance gaps between global and project hits.

This carries forward the PR #43 fixes while dropping the extra
cross-DB score rescaling step that could flatten real relevance
gaps between global and project search hits. Keeping the merged
order on raw final scores preserves the branch intent without
leaving the Gemini review concern in place.

Constraint: Current workspace has unrelated local changes, so the PR was rebuilt and fixed in an isolated worktree
Rejected: Per-scope max renormalization | it can promote weak project hits to parity with stronger global hits
Rejected: Keep the block and micro-optimize max lookup | preserves disputed ranking behavior
Confidence: medium
Scope-risk: narrow
Reversibility: clean
Directive: Do not add per-store score rescaling back without proving the merged ranking semantics with tests
Tested: cargo check -p memory-server
Tested: cargo test -p memory-server -- --nocapture
Not-tested: Dedicated regression test for cross-store ranking semantics
Related: PR #43
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