fix(mcp): resolve project_id by name for MCP clients#55
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds project identifier normalization: Server gains resolveProjectID to prefer direct ID matches then name-based lookup. Store gains ResolveProjectByName and MemoryStore interface is updated; MCP handlers use the resolver before memory operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as MCP Handler
participant Server
participant Store
participant DB
Client->>MCP: request with ProjectID (id or name)
MCP->>Server: call handler (args.ProjectID)
Server->>Server: resolveProjectID(ctx, input)
alt input matches existing project ID
Server->>Store: ListProjects or direct check for ID
Store-->>Server: found ID
else no direct ID match
Server->>Store: ResolveProjectByName(ctx, name)
Store->>DB: query "SELECT id FROM projects WHERE name = ?"
DB-->>Store: id or sql.ErrNoRows
Store-->>Server: id or ""
end
Server-->>MCP: normalized ProjectID
MCP->>Store: perform memory operation with normalized ID
Store-->>MCP: operation result
MCP-->>Client: response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
🤖 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 147-150: The call to s.store.EnsureProject passes an empty path
(s.store.EnsureProject(ctx, args.ProjectID, "", args.ProjectID)), which will
overwrite existing project.path with "" due to the ON CONFLICT update; fix by
first fetching the existing project path (e.g., via s.store.GetProject(ctx,
args.ProjectID) or equivalent) and pass that path (or nil/unchanged value) into
EnsureProject instead of "", i.e., replace the "" argument with the current
project's Path when present so EnsureProject does not clobber the real path.
- Around line 50-72: The resolveProjectID function currently swallows store
errors; change its signature to return (string, error) and propagate backend
errors instead of returning the raw input on failure: call
s.store.ListProjects(ctx) and if it returns an error return "", err; otherwise
check IDs and return input,nil if matched; then call
s.store.ResolveProjectByName(ctx,input) and if it returns an error return "",
err; if resolved != "" return resolved,nil; if nothing matched return input,nil.
Update all callers of resolveProjectID to handle the (string, error) return and
propagate or handle errors accordingly.
In `@internal/memory/store.go`:
- Around line 107-113: The current QueryRowContext call in the project name
resolution can return an arbitrary ID because projects.name is not unique;
change the lookup to fetch all matching rows (use s.db.QueryContext or Query)
and collect IDs, then: if zero rows return "", nil; if more than one row return
a clear, non-nil error indicating an ambiguous/duplicate project name (e.g.,
"multiple projects with name %q"); only when exactly one ID is found return that
ID. Update the error handling around the existing s.db.QueryRowContext usage to
implement this multi-row check and return semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5826c6a-f1ac-4a63-89b6-ad2adec0aefa
📒 Files selected for processing (4)
internal/mcpserver/mcpserver.gointernal/memory/store.gointernal/provider/provider.gointernal/server/server_test.go
Comment on lines
+50
to
+72
| // resolveProjectID resolves a project_id that may be a name (e.g. "ghost") | ||
| // into the actual hash ID (e.g. "6bdc098af7f5") stored in the database. | ||
| // Tries direct ID match first, then falls back to name lookup. | ||
| func (s *Server) resolveProjectID(ctx context.Context, input string) string { | ||
| // Check if input directly matches a project ID. | ||
| projects, err := s.store.ListProjects(ctx) | ||
| if err == nil { | ||
| for _, p := range projects { | ||
| if p.ID == input { | ||
| return input | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Try name lookup. | ||
| resolved, err := s.store.ResolveProjectByName(ctx, input) | ||
| if err == nil && resolved != "" { | ||
| return resolved | ||
| } | ||
|
|
||
| // Return as-is if nothing matched — let downstream handle the miss. | ||
| return input | ||
| } |
There was a problem hiding this comment.
Don’t suppress project-resolution errors.
Line [55] and Line [65] ignore store errors and fall through to returning input, which can hide backend failures and produce wrong project targeting instead of failing fast.
Proposed fix
-func (s *Server) resolveProjectID(ctx context.Context, input string) string {
+func (s *Server) resolveProjectID(ctx context.Context, input string) (string, error) {
// Check if input directly matches a project ID.
projects, err := s.store.ListProjects(ctx)
- if err == nil {
- for _, p := range projects {
- if p.ID == input {
- return input
- }
- }
+ if err != nil {
+ return "", fmt.Errorf("list projects: %w", err)
+ }
+ for _, p := range projects {
+ if p.ID == input {
+ return input, nil
+ }
}
// Try name lookup.
resolved, err := s.store.ResolveProjectByName(ctx, input)
- if err == nil && resolved != "" {
- return resolved
+ if err != nil {
+ return "", fmt.Errorf("resolve project by name: %w", err)
+ }
+ if resolved != "" {
+ return resolved, nil
}
// Return as-is if nothing matched — let downstream handle the miss.
- return input
+ return input, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcpserver/mcpserver.go` around lines 50 - 72, The resolveProjectID
function currently swallows store errors; change its signature to return
(string, error) and propagate backend errors instead of returning the raw input
on failure: call s.store.ListProjects(ctx) and if it returns an error return "",
err; otherwise check IDs and return input,nil if matched; then call
s.store.ResolveProjectByName(ctx,input) and if it returns an error return "",
err; if resolved != "" return resolved,nil; if nothing matched return input,nil.
Update all callers of resolveProjectID to handle the (string, error) return and
propagate or handle errors accordingly.
Comment on lines
+147
to
150
| args.ProjectID = s.resolveProjectID(ctx, args.ProjectID) | ||
|
|
||
| if err := s.store.EnsureProject(ctx, args.ProjectID, "", args.ProjectID); err != nil { | ||
| return nil, nil, fmt.Errorf("ensure project: %w", err) |
There was a problem hiding this comment.
EnsureProject with empty path can corrupt existing project metadata.
Line [149] passes path="". In internal/memory/store.go Line [87], ON CONFLICT(id) DO UPDATE SET path = excluded.path will overwrite real paths with empty string and can later trigger UNIQUE(path) collisions.
Proposed fix
-args.ProjectID = s.resolveProjectID(ctx, args.ProjectID)
+originalProjectID := args.ProjectID
+args.ProjectID, err = s.resolveProjectID(ctx, args.ProjectID)
+if err != nil {
+ return nil, nil, fmt.Errorf("resolve project_id: %w", err)
+}
-if err := s.store.EnsureProject(ctx, args.ProjectID, "", args.ProjectID); err != nil {
- return nil, nil, fmt.Errorf("ensure project: %w", err)
+// Do not overwrite existing project metadata with empty path.
+if args.ProjectID == originalProjectID {
+ if err := s.store.EnsureProject(ctx, args.ProjectID, "", args.ProjectID); err != nil {
+ return nil, nil, fmt.Errorf("ensure project: %w", err)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcpserver/mcpserver.go` around lines 147 - 150, The call to
s.store.EnsureProject passes an empty path (s.store.EnsureProject(ctx,
args.ProjectID, "", args.ProjectID)), which will overwrite existing project.path
with "" due to the ON CONFLICT update; fix by first fetching the existing
project path (e.g., via s.store.GetProject(ctx, args.ProjectID) or equivalent)
and pass that path (or nil/unchanged value) into EnsureProject instead of "",
i.e., replace the "" argument with the current project's Path when present so
EnsureProject does not clobber the real path.
Comment on lines
+107
to
+113
| err := s.db.QueryRowContext(ctx, `SELECT id FROM projects WHERE name = ? LIMIT 1`, name).Scan(&id) | ||
| if err == sql.ErrNoRows { | ||
| return "", nil | ||
| } | ||
| if err != nil { | ||
| return "", fmt.Errorf("resolve project by name: %w", err) | ||
| } |
There was a problem hiding this comment.
Handle duplicate project names explicitly to avoid wrong-ID resolution.
Line [107] uses WHERE name = ? LIMIT 1, but projects.name is not unique. This can return an arbitrary ID when duplicates exist.
Proposed fix
func (s *Store) ResolveProjectByName(ctx context.Context, name string) (string, error) {
s.mu.RLock()
defer s.mu.RUnlock()
- var id string
- err := s.db.QueryRowContext(ctx, `SELECT id FROM projects WHERE name = ? LIMIT 1`, name).Scan(&id)
- if err == sql.ErrNoRows {
- return "", nil
- }
- if err != nil {
- return "", fmt.Errorf("resolve project by name: %w", err)
- }
- return id, nil
+ rows, err := s.db.QueryContext(ctx, `
+ SELECT id
+ FROM projects
+ WHERE name = ?
+ ORDER BY updated_at DESC
+ LIMIT 2
+ `, name)
+ if err != nil {
+ return "", fmt.Errorf("resolve project by name: %w", err)
+ }
+ defer rows.Close()
+
+ var ids []string
+ for rows.Next() {
+ var id string
+ if err := rows.Scan(&id); err != nil {
+ return "", fmt.Errorf("resolve project by name scan: %w", err)
+ }
+ ids = append(ids, id)
+ }
+ if err := rows.Err(); err != nil {
+ return "", fmt.Errorf("resolve project by name rows: %w", err)
+ }
+ if len(ids) == 0 {
+ return "", nil
+ }
+ if len(ids) > 1 {
+ return "", fmt.Errorf("resolve project by name: ambiguous project name %q", name)
+ }
+ return ids[0], nil
}📝 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.
Suggested change
| err := s.db.QueryRowContext(ctx, `SELECT id FROM projects WHERE name = ? LIMIT 1`, name).Scan(&id) | |
| if err == sql.ErrNoRows { | |
| return "", nil | |
| } | |
| if err != nil { | |
| return "", fmt.Errorf("resolve project by name: %w", err) | |
| } | |
| func (s *Store) ResolveProjectByName(ctx context.Context, name string) (string, error) { | |
| s.mu.RLock() | |
| defer s.mu.RUnlock() | |
| rows, err := s.db.QueryContext(ctx, ` | |
| SELECT id | |
| FROM projects | |
| WHERE name = ? | |
| ORDER BY updated_at DESC | |
| LIMIT 2 | |
| `, name) | |
| if err != nil { | |
| return "", fmt.Errorf("resolve project by name: %w", err) | |
| } | |
| defer rows.Close() | |
| var ids []string | |
| for rows.Next() { | |
| var id string | |
| if err := rows.Scan(&id); err != nil { | |
| return "", fmt.Errorf("resolve project by name scan: %w", err) | |
| } | |
| ids = append(ids, id) | |
| } | |
| if err := rows.Err(); err != nil { | |
| return "", fmt.Errorf("resolve project by name rows: %w", err) | |
| } | |
| if len(ids) == 0 { | |
| return "", nil | |
| } | |
| if len(ids) > 1 { | |
| return "", fmt.Errorf("resolve project by name: ambiguous project name %q", name) | |
| } | |
| return ids[0], nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/memory/store.go` around lines 107 - 113, The current QueryRowContext
call in the project name resolution can return an arbitrary ID because
projects.name is not unique; change the lookup to fetch all matching rows (use
s.db.QueryContext or Query) and collect IDs, then: if zero rows return "", nil;
if more than one row return a clear, non-nil error indicating an
ambiguous/duplicate project name (e.g., "multiple projects with name %q"); only
when exactly one ID is found return that ID. Update the error handling around
the existing s.db.QueryRowContext usage to implement this multi-row check and
return semantics.
MCP clients pass project name ("ghost") but DB stores project_id as
sha256(abs_path)[:12]. Every MCP query returned empty results.
Add ResolveProjectByName to Store and resolveProjectID helper to MCP
server. All tool handlers now resolve name→hash before querying.
341170b to
2972ace
Compare
wcatz
added a commit
that referenced
this pull request
Mar 16, 2026
Surgical scope lock: Ghost is a personal assistant, not a coding agent.
Removed (coding tools only):
- internal/tool/{bash,file_edit,file_read,file_write,git,glob,grep}.go
- Simplified modes to chat only, default mode -> chat
- Updated personality prompt to "personal assistant"
Kept intact:
- VSCode extension (vscode-ghost/) — assistant frontend
- Voice pipeline (internal/voice/) — Phase C
- TUI with all features — toolbar, approval, images, cost tracking
- Telegram bot — all commands, approval forwarding
- Google Calendar + Gmail, GitHub notifications, scheduler, briefing
Added (cherry-picked fixes):
- MCP: resolveProjectID for name->hash lookup (from PR #55)
- Security: Telegram bot sends bearer token on API calls
- Security: SSE approval goroutine respects context cancellation
- Security: scheduler randomID returns error instead of panicking
- Security: GitHub monitor handles crypto/rand failure
8 tasks
wcatz
added a commit
that referenced
this pull request
Mar 16, 2026
…ds (#58) * refactor: remove coding tools, fix MCP + security, keep all frontends Surgical scope lock: Ghost is a personal assistant, not a coding agent. Removed (coding tools only): - internal/tool/{bash,file_edit,file_read,file_write,git,glob,grep}.go - Simplified modes to chat only, default mode -> chat - Updated personality prompt to "personal assistant" Kept intact: - VSCode extension (vscode-ghost/) — assistant frontend - Voice pipeline (internal/voice/) — Phase C - TUI with all features — toolbar, approval, images, cost tracking - Telegram bot — all commands, approval forwarding - Google Calendar + Gmail, GitHub notifications, scheduler, briefing Added (cherry-picked fixes): - MCP: resolveProjectID for name->hash lookup (from PR #55) - Security: Telegram bot sends bearer token on API calls - Security: SSE approval goroutine respects context cancellation - Security: scheduler randomID returns error instead of panicking - Security: GitHub monitor handles crypto/rand failure * fix: address CodeRabbit review findings - resolveProjectID: name lookup first to avoid hash collisions - schema: conversations.mode default 'code' → 'chat' - prompt: add sensitive-data guard for memory_save
This was referenced Mar 16, 2026
This was referenced Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
project_id, but Ghost's DB stores project IDs assha256(abs_path)[:12](e.g. "6bdc098af7f5"). Every MCP query returned empty results.ResolveProjectByNametomemory.Store— simpleSELECT id FROM projects WHERE name = ?resolveProjectIDhelper to MCP server — tries direct ID match first, falls back to name lookupsearch,save,context,list) now resolve before queryingTest plan
go vet ./...— cleango test ./...— all passghost mcp→ callghost_memory_searchwithproject_id="ghost"→ returns actual memoriesghost_memory_savewith project name creates memory under correct hash IDSummary by CodeRabbit