feat(mcp): support project reassignment in mem_update - #679
feat(mcp): support project reassignment in mem_update#679Faturrachman-dev wants to merge 1 commit into
Conversation
mem_update accepted title/content/type/scope/topic_key but not project, so an observation saved under the wrong project could never be moved — the only recourse was raw SQL or an export/edit/reimport round-trip. The store layer already supported this: UpdateObservationParams has a Project field, UpdateObservation writes it (with NormalizeProject), and handleUpdate's guard already referenced update.Project. Only the tool schema and the argument parse were missing. This wires both and adds a store-level test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe ChangesObservation project reassignment
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant handleUpdate
participant UpdateObservation
MCPClient->>handleUpdate: mem_update with project
handleUpdate->>UpdateObservation: update.Project
UpdateObservation-->>handleUpdate: updated observation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/store/store_test.go`:
- Around line 8834-8872: Extend TestUpdateObservationReassignsProject to cover
UpdateObservation error handling for an invalid or missing observation ID, plus
deterministic assertions for empty project input and the intended project-name
normalization behavior before merge. Keep the existing successful reassignment
and persistence checks, and use the store’s established error and normalization
expectations rather than inventing new behavior.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 588bb4d5-e193-4d21-b742-99c292312a69
📒 Files selected for processing (2)
internal/mcp/mcp.gointernal/store/store_test.go
| func TestUpdateObservationReassignsProject(t *testing.T) { | ||
| s := newTestStore(t) | ||
|
|
||
| if err := s.CreateSession("s1", "alpha", "/tmp/alpha"); err != nil { | ||
| t.Fatalf("create session: %v", err) | ||
| } | ||
|
|
||
| id, err := s.AddObservation(AddObservationParams{ | ||
| SessionID: "s1", | ||
| Type: "config", | ||
| Title: "movable", | ||
| Content: "belongs elsewhere", | ||
| Project: "alpha", | ||
| Scope: "project", | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("add observation: %v", err) | ||
| } | ||
|
|
||
| newProject := "beta" | ||
| updated, err := s.UpdateObservation(id, UpdateObservationParams{ | ||
| Project: &newProject, | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("update observation: %v", err) | ||
| } | ||
| if derefString(updated.Project) != "beta" { | ||
| t.Fatalf("project reassignment did not apply; got project=%q, want %q", derefString(updated.Project), "beta") | ||
| } | ||
|
|
||
| // Confirm it persisted on re-read. | ||
| got, err := s.GetObservation(id) | ||
| if err != nil { | ||
| t.Fatalf("get observation: %v", err) | ||
| } | ||
| if derefString(got.Project) != "beta" { | ||
| t.Fatalf("reassignment not persisted; got project=%q, want %q", derefString(got.Project), "beta") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Extend coverage beyond the happy path.
This test verifies successful reassignment and persistence, but omits UpdateObservation error paths and project-input edge cases. Add deterministic assertions for an invalid/missing observation ID and the intended empty/normalization behavior before merge.
As per path instructions, **/*_test.go must verify happy path, error paths, and edge cases, and behavior changes without tests should be blocked.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/store_test.go` around lines 8834 - 8872, Extend
TestUpdateObservationReassignsProject to cover UpdateObservation error handling
for an invalid or missing observation ID, plus deterministic assertions for
empty project input and the intended project-name normalization behavior before
merge. Keep the existing successful reassignment and persistence checks, and use
the store’s established error and normalization expectations rather than
inventing new behavior.
Source: Path instructions
There was a problem hiding this comment.
Pull request overview
This PR extends the MCP mem_update tool so callers can reassign an existing observation to a different project, matching capabilities already present in the store layer. It also adds a store-level test to confirm project reassignment persists.
Changes:
- Add a
projectstring parameter to the MCPmem_updatetool schema. - Parse the
projectargument inhandleUpdateand forward it viastore.UpdateObservationParams. - Add
TestUpdateObservationReassignsProjectto validate reassignment and persistence on re-read.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/mcp/mcp.go | Exposes and parses project in mem_update so MCP callers can reassign an observation’s project. |
| internal/store/store_test.go | Adds a regression test ensuring UpdateObservation can change (and persist) an observation’s project. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if v, ok := req.GetArguments()["project"].(string); ok { | ||
| update.Project = &v | ||
| } |
| newProject := "beta" | ||
| updated, err := s.UpdateObservation(id, UpdateObservationParams{ | ||
| Project: &newProject, | ||
| }) |
Problem
mem_updateexposestitle,content,type,scope, andtopic_key, but notproject. So an observation saved under the wrong project can never be moved through the public API — the only recourse is raw SQL againstengram.dbor a full export → edit → reimport round-trip. There's no per-observation reassignment anywhere (CLI, MCP, or HTTP);projects/migrateandconsolidateoperate at whole-project granularity.This is easy to hit in practice: when the working directory resolves to a fallback/default project, a batch of observations lands in the wrong bucket and there's no supported way to reclassify them individually.
Fix
The backend already supports this — the missing piece was only the MCP surface:
store.UpdateObservationParamsalready has aProject *stringfield.store.UpdateObservationalready writes it (viaNormalizeProject).handleUpdate's "nothing to update" guard already referencesupdate.Project.Only the tool schema and the argument parse were absent. This PR:
mcp.WithString("project", ...)to themem_updatetool definition.handleUpdateintoupdate.Project.TestUpdateObservationReassignsProjectcovering reassign + persisted re-read.Test
+46 lines, no behavior change to existing fields.
🤖 Generated with Claude Code
Summary by CodeRabbit