fix(mcp): audit fixes — ownership check, panic guard, partial update, hybrid search#146
fix(mcp): audit fixes — ownership check, panic guard, partial update, hybrid search#146
Conversation
… hybrid search Addresses 11 of 13 findings from MCP server audit: 🔴 Reds (crashes / data loss): - ghost_health: p.ID[:8] panics on _global (7 chars) → min(len, 8) - ghost_memory_delete: no ownership boundary → requires project_id, verifies via GetByIDs - ghost_task_update: Priority int+omitempty silently clobbers to 0 → *int/*string + fetch current task 🟠 Correctness: - ghost_task_update: omitting description wiped the field → fetch-and-merge via new GetTask store method - ghost_search_all: FTS-only even when embedder available → SearchHybridAll (RRF) added to store - Resource URI handlers: copy-pasted 3×, no PathUnescape → parseProjectIDFromURI helper 🟡 Quality / Design: - EnsureProject path arg was project name, not fs path → pass "" for MCP-created projects - ghost_memory_save / ghost_save_global: no content length limit → 2000-char cap, truncation reported - ghost_task_list: hardcoded limit=30 → configurable Limit field (default 30, max 100) - formatMemories: json.Marshal result silently discarded → check err before use - ghost_memory_search description: category filter truncation undocumented → documented Store additions: GetTask, SearchVectorAll, SearchHybridAll (all wired into MemoryStore interface). Deferred: resolveProjectID O(n) scan (#6), registerTools monolith (#13).
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR enhances memory and task management by implementing hybrid search (FTS + vector), adding task retrieval capabilities, refactoring URI parsing in MCP handlers, introducing optional task update fields, capping memory content to 2000 characters, improving category filtering, and adjusting tool schemas for better validation. Changes
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Handler as ghost_memory_search Handler
participant FTS as FTS Engine
participant Vector as Vector Search
participant Store as Memory Store
participant Memory as Memory Objects
Client->>Handler: search(query, queryVec, category, limit)
Handler->>FTS: SearchFTSAll(query)
FTS-->>Handler: fts_results
Handler->>Vector: SearchVectorAll(queryVec)
Vector-->>Handler: vector_results
Handler->>Handler: RRF Fusion (0.3 FTS, 0.7 Vector)
Handler->>Handler: Post-filter by category<br/>(retrieve limit*3, filter, truncate to limit)
Handler->>Store: GetByIDs(memory_ids)
Store->>Memory: Query memories
Memory-->>Store: memory_objects
Store-->>Handler: memories
Handler-->>Client: ranked_memories
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/mcpserver/mcpserver.go (1)
647-647:⚠️ Potential issue | 🟡 MinorApply the same panic guard to
t.ID[:8]that you added toghost_health.Line 746 now uses
p.ID[:min(len(p.ID), 8)]to avoid the 7-char slicing panic, but the equivalent task-ID slicing at line 647 (ghost_task_list) and line 1060 (tasks resource) still doest.ID[:8]unconditionally. If task IDs are always DB-generated UUIDs this is latent, but defensive consistency with the health-tool fix avoids a future regression if the ID format ever changes (or if callers surface a shortened ID).🛡️ Proposed fix (apply to both call sites)
- fmt.Fprintf(&sb, "- [%s] P%d `%s` %s\n", t.Status, t.Priority, t.ID[:8], t.Title) + fmt.Fprintf(&sb, "- [%s] P%d `%s` %s\n", t.Status, t.Priority, t.ID[:min(len(t.ID), 8)], t.Title)Also applies to: 1060-1060
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcpserver/mcpserver.go` at line 647, The slice t.ID[:8] can panic for short IDs; change the formatting in ghost_task_list (the fmt.Fprintf call that prints t.ID[:8]) to guard against short IDs by slicing with the minimum of len(t.ID) and 8 (e.g., use t.ID[:min(len(t.ID),8)] or a small helper like shortID(t.ID,8)), matching the defensive fix used for p.ID in ghost_health; apply the same change to the other tasks resource site referenced so both places use the safe slice.
🧹 Nitpick comments (3)
internal/mcpserver/mcpserver.go (2)
441-466: Ownership check looks correct; consider documenting_globalhandling.The
GetByIDs+ProjectIDcompare is the right shape, andresolveProjectIDallows deleting_globalmemories when the caller passes"_global". Worth a one-line mention in the tool description so clients know how to delete a global memory, since the description currently only talks about project-scoped ownership.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcpserver/mcpserver.go` around lines 441 - 466, Add a brief note to the tool's user-facing description explaining that resolveProjectID accepts "_global" and that calling the delete memory tool with ProjectID set to "_global" will target global memories (the code path using s.resolveProjectID, s.store.GetByIDs and the ProjectID ownership check already permits deleting global memories); update the tool description text for the delete memory tool to mention how to delete a global memory so clients know to pass "_global".
192-197: Minor: Embed errors are silently swallowed.When
s.embedder.Embedfails,queryVecstaysniland the caller silently falls back to FTS-only. This is the intended behavior, but logging at debug level would make embedder misconfigurations (wrong model, network errors) observable without changing control flow.💡 Suggested logging
if s.embedder != nil { - if vec, err := s.embedder.Embed(ctx, args.Query); err == nil { + if vec, err := s.embedder.Embed(ctx, args.Query); err != nil { + s.logger.Debug("embed failed, falling back to FTS", "err", err) + } else { queryVec = vec } }Also applies to: 492-497
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcpserver/mcpserver.go` around lines 192 - 197, The embedder error is being swallowed when calling s.embedder.Embed (leaving queryVec nil); instead, when Embed returns an error, log the error at debug level so misconfigurations are observable without changing behavior. Update the blocks around s.embedder.Embed where queryVec is set (the snippet using s.embedder.Embed and the similar block later) to check err != nil and call the server's logger (e.g., s.logger.Debugf or s.log.Debugf, whichever the struct uses) with a concise message including the error and that embedding failed, then continue leaving queryVec nil as before.internal/memory/vector.go (1)
284-350: Recommended refactor: factor RRF fusion into a shared helper.
SearchHybridAllduplicates ~60 lines fromSearchHybrid(lines 133-212) — only the per-project vs. all-project retrieval differs. A small helper (e.g.fuseRRF(ftsResults []Memory, vecResults []ScoredMemory, limit int) []idScore) plus a shared post-fetch loader would eliminate two near-identical code paths and keep the 0.3/0.7/k=60 constants in one place. Deferrable, but worthwhile before this file grows another variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/memory/vector.go` around lines 284 - 350, SearchHybridAll duplicates the RRF fusion and post-fetch sorting logic present in SearchHybrid; extract that logic into a shared helper (e.g. fuseRRF(ftsResults []Memory, vecResults []ScoredMemory, limit int) []idScore) that takes the FTS and vector results plus limit and returns the ranked idScore slice (using the same k/weights constants), then add a small shared loader (e.g. loadAndSortByScores(ctx, ids []string, scores map[string]float64) ([]Memory, error)) that calls GetByIDs and reorders by the score map; update both SearchHybridAll and SearchHybrid to call fuseRRF and the loader so the RRF weights (0.3/0.7 and k=60) and post-fetch sorting live in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/mcpserver/mcpserver.go`:
- Around line 852-856: The error returned from s.store.GetTask is wrapped with
"task not found" regardless of cause; change the handling in the block that
calls s.store.GetTask(ctx, args.TaskID) to distinguish sql.ErrNoRows from other
DB errors by using errors.Is(err, sql.ErrNoRows) and return fmt.Errorf("task not
found: %w", err) only in that case, otherwise return a neutral wrapper such as
fmt.Errorf("lookup task: %w", err); ensure errors is imported and reference the
variables/function names s.store.GetTask, current, err when making the change.
In `@internal/memory/vector.go`:
- Line 255: The deferred rows.Close() return value must be handled to satisfy
errcheck; replace the bare defer rows.Close() with an explicit discard or
error-handling closure (e.g. defer func(){ _ = rows.Close() }() or defer func(){
if err := rows.Close(); err != nil { /* log or ignore */ } }()) in the function
that contains rows (the same pattern used by SearchVector); ensure you reference
rows.Close() directly so the linter sees the return value handled.
---
Outside diff comments:
In `@internal/mcpserver/mcpserver.go`:
- Line 647: The slice t.ID[:8] can panic for short IDs; change the formatting in
ghost_task_list (the fmt.Fprintf call that prints t.ID[:8]) to guard against
short IDs by slicing with the minimum of len(t.ID) and 8 (e.g., use
t.ID[:min(len(t.ID),8)] or a small helper like shortID(t.ID,8)), matching the
defensive fix used for p.ID in ghost_health; apply the same change to the other
tasks resource site referenced so both places use the safe slice.
---
Nitpick comments:
In `@internal/mcpserver/mcpserver.go`:
- Around line 441-466: Add a brief note to the tool's user-facing description
explaining that resolveProjectID accepts "_global" and that calling the delete
memory tool with ProjectID set to "_global" will target global memories (the
code path using s.resolveProjectID, s.store.GetByIDs and the ProjectID ownership
check already permits deleting global memories); update the tool description
text for the delete memory tool to mention how to delete a global memory so
clients know to pass "_global".
- Around line 192-197: The embedder error is being swallowed when calling
s.embedder.Embed (leaving queryVec nil); instead, when Embed returns an error,
log the error at debug level so misconfigurations are observable without
changing behavior. Update the blocks around s.embedder.Embed where queryVec is
set (the snippet using s.embedder.Embed and the similar block later) to check
err != nil and call the server's logger (e.g., s.logger.Debugf or s.log.Debugf,
whichever the struct uses) with a concise message including the error and that
embedding failed, then continue leaving queryVec nil as before.
In `@internal/memory/vector.go`:
- Around line 284-350: SearchHybridAll duplicates the RRF fusion and post-fetch
sorting logic present in SearchHybrid; extract that logic into a shared helper
(e.g. fuseRRF(ftsResults []Memory, vecResults []ScoredMemory, limit int)
[]idScore) that takes the FTS and vector results plus limit and returns the
ranked idScore slice (using the same k/weights constants), then add a small
shared loader (e.g. loadAndSortByScores(ctx, ids []string, scores
map[string]float64) ([]Memory, error)) that calls GetByIDs and reorders by the
score map; update both SearchHybridAll and SearchHybrid to call fuseRRF and the
loader so the RRF weights (0.3/0.7 and k=60) and post-fetch sorting live in one
place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddca4d4c-5dd9-42af-9286-2b2638d8dcd3
📒 Files selected for processing (5)
internal/mcpserver/mcpserver.gointernal/memory/tasks.gointernal/memory/vector.gointernal/provider/provider.gointernal/server/server_test.go
| // Fetch current task to fill in omitted fields (prevents zero-value clobber). | ||
| current, err := s.store.GetTask(ctx, args.TaskID) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("task not found: %w", err) | ||
| } |
There was a problem hiding this comment.
Nit: misleading error wrap.
GetTask can fail with sql.ErrNoRows (truly not found) or a transient DB error; wrapping unconditionally as "task not found: %w" will mislead callers when it's the latter. Either differentiate on errors.Is(err, sql.ErrNoRows) or use a neutral prefix like "lookup task: %w".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcpserver/mcpserver.go` around lines 852 - 856, The error returned
from s.store.GetTask is wrapped with "task not found" regardless of cause;
change the handling in the block that calls s.store.GetTask(ctx, args.TaskID) to
distinguish sql.ErrNoRows from other DB errors by using errors.Is(err,
sql.ErrNoRows) and return fmt.Errorf("task not found: %w", err) only in that
case, otherwise return a neutral wrapper such as fmt.Errorf("lookup task: %w",
err); ensure errors is imported and reference the variables/function names
s.store.GetTask, current, err when making the change.
| if err != nil { | ||
| return nil, fmt.Errorf("load embeddings: %w", err) | ||
| } | ||
| defer rows.Close() |
There was a problem hiding this comment.
Blocker: CI errcheck failure — handle rows.Close() return.
golangci-lint fails the build here. Although SearchVector uses the same bare-defer pattern, the linter is flagging new code; the minimal fix is to explicitly discard (or log) the close error.
🔧 Proposed fix
- defer rows.Close()
+ defer func() { _ = rows.Close() }()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defer rows.Close() | |
| defer func() { _ = rows.Close() }() |
🧰 Tools
🪛 GitHub Actions: ci
[error] 255-255: golangci-lint (errcheck): Error return value of rows.Close is not checked.
🪛 GitHub Check: build-and-test
[failure] 255-255:
Error return value of rows.Close is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/memory/vector.go` at line 255, The deferred rows.Close() return
value must be handled to satisfy errcheck; replace the bare defer rows.Close()
with an explicit discard or error-handling closure (e.g. defer func(){ _ =
rows.Close() }() or defer func(){ if err := rows.Close(); err != nil { /* log or
ignore */ } }()) in the function that contains rows (the same pattern used by
SearchVector); ensure you reference rows.Close() directly so the linter sees the
return value handled.
Bumps [google.golang.org/api](https://github.com/googleapis/google-api-go-client) from 0.272.0 to 0.275.0. - [Release notes](https://github.com/googleapis/google-api-go-client/releases) - [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md) - [Commits](googleapis/google-api-go-client@v0.272.0...v0.275.0) --- updated-dependencies: - dependency-name: google.golang.org/api dependency-version: 0.275.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
Full MCP server audit pass — 11 of 13 findings fixed across correctness, safety, and quality categories.
🔴 Critical (crashes / data loss):
ghost_healthpanics on_globalproject (7-char ID,[:8]slice) →min(len, 8)ghost_memory_deletehad no project ownership check — any agent could delete any memory by ID → now requiresproject_id, verifies viaGetByIDsbefore deleteghost_task_updateusedPriority intwithomitempty— omitting priority silently wrote0(critical) to DB → changed to*int/*stringwith fetch-and-merge pattern via newGetTask🟠 Correctness:
ghost_task_updateomittingdescriptionwiped the existing value — same fetch-and-merge fixghost_search_allwas FTS-only even when embedder is configured → addedSearchHybridAll(RRF fusion) to store, wired into the toolurl.PathUnescape→ extractedparseProjectIDFromURIhelper used in all three🟡 Quality / Design:
EnsureProjectwas called with the project name as the path arg → now passes""(MCP callers have no FS path)ghost_memory_saveandghost_save_global, truncation reported in responseghost_task_listhad hardcodedlimit=30with no way for callers to control it →Limitfield added (default 30, max 100)json.Marshalresult informatMemorieswas silently discarded → now checkedghost_memory_searchdescription → documented with recommendation to useghost_memories_listfor exhaustive browsingStore additions:
GetTask(ctx, taskID) (Task, error)— used by partial updateSearchVectorAll— brute-force cosine across all projectsSearchHybridAll— RRF fusion across all projects (FTS + vector)MemoryStoreinterface;verify.gocompile-time check catches any missed implementationsDeferred:
resolveProjectIDO(n) scan on every tool call (chore(deps): bump github.com/mattn/go-sqlite3 from 1.14.28 to 1.14.34 #6) — needsGetProjectByIDin storeregisterTools500-line monolith (fix(reflection): guard against empty-content and dramatic memory reduction #13) — style-only, no correctness changeTest plan
go vet ./...cleango test ./...all pass (27 packages)ghost_memory_deletewith wrong project_id returns ownership errorghost_task_updatestatus-only call preserves existing priority and descriptionghost_healthwith_globalproject present (no panic)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes