fix(mcp): track save freshness by project - #702
Draft
sparkiesweden wants to merge 1 commit into
Draft
Conversation
📝 WalkthroughWalkthroughChangesProject activity nudges
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MCPHandler
participant SessionActivity
participant ProjectStore
MCPHandler->>ProjectStore: Resolve project
MCPHandler->>SessionActivity: NudgeForProject(sessionID, project)
SessionActivity-->>MCPHandler: Return project-scoped nudge
MCPHandler->>SessionActivity: RecordProjectSave(sessionID, project)
SessionActivity-->>MCPHandler: Update session and project freshness
🚥 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/mcp/activity_test.go`:
- Around line 82-123: Add deterministic empty-project tests covering
RecordProjectSave and RecordProjectFreshness. Verify projectSaves[""] is not
created, project nudges remain unsuppressed, and RecordProjectSave still updates
the specified session’s ActivityScore; retain the existing nonempty-project
coverage.
🪄 Autofix
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: 9ea9877c-84f8-4650-8ff9-26775991fcea
📒 Files selected for processing (4)
internal/mcp/activity.gointernal/mcp/activity_test.gointernal/mcp/mcp.gointernal/mcp/mcp_test.go
Comment on lines
+82
to
+123
| func TestSessionActivity_ProjectSaveSuppressesNudgeWithoutMutatingDefaultScore(t *testing.T) { | ||
| now := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) | ||
| a := NewSessionActivity(10 * time.Minute) | ||
| a.now = func() time.Time { return now } | ||
|
|
||
| project := "myproject" | ||
| defaultSID := defaultSessionID(project) | ||
| for i := 0; i < 6; i++ { | ||
| a.RecordToolCall(defaultSID) | ||
| } | ||
| now = now.Add(15 * time.Minute) | ||
| if nudge := a.NudgeForProject(defaultSID, project); nudge == "" { | ||
| t.Fatal("expected project nudge before save") | ||
| } | ||
|
|
||
| a.RecordProjectSave("explicit-session", project) | ||
| if nudge := a.NudgeForProject(defaultSID, project); nudge != "" { | ||
| t.Fatalf("expected project save to suppress nudge, got %q", nudge) | ||
| } | ||
| if score := a.ActivityScore(defaultSID); !strings.Contains(score, "0 saves") { | ||
| t.Fatalf("expected default session score to remain unchanged, got %q", score) | ||
| } | ||
| } | ||
|
|
||
| func TestSessionActivity_NudgeForProjectExpiresOldProjectSave(t *testing.T) { | ||
| now := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) | ||
| a := NewSessionActivity(10 * time.Minute) | ||
| a.now = func() time.Time { return now } | ||
| project := "myproject" | ||
| defaultSID := defaultSessionID(project) | ||
| for i := 0; i < 6; i++ { | ||
| a.RecordToolCall(defaultSID) | ||
| } | ||
| a.RecordProjectSave("explicit-session", project) | ||
| now = now.Add(15 * time.Minute) | ||
| if nudge := a.NudgeForProject(defaultSID, project); nudge == "" { | ||
| t.Fatal("expected nudge after project save freshness expires") | ||
| } | ||
| if _, ok := a.projectSaves[project]; ok { | ||
| t.Fatal("expected expired project save entry to be removed") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the empty-project branch.
The added tests use only a nonempty project. Add deterministic tests for RecordProjectSave and RecordProjectFreshness with project == "". Verify that neither method creates projectSaves[""] or suppresses a project nudge. Verify that RecordProjectSave still updates the specified session score.
As per path instructions, "**/*_test.go: Verify coverage of happy path, error paths, and edge cases."
🤖 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/mcp/activity_test.go` around lines 82 - 123, Add deterministic
empty-project tests covering RecordProjectSave and RecordProjectFreshness.
Verify projectSaves[""] is not created, project nudges remain unsuppressed, and
RecordProjectSave still updates the specified session’s ActivityScore; retain
the existing nonempty-project coverage.
Source: Path instructions
sparkiesweden
marked this pull request as draft
August 6, 2026 18:32
2 tasks
Author
|
Process update: dedicated issue #703 is open and this PR is now draft pending its |
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
mem_search/mem_contextwarnings aftermem_saveormem_session_summaryRoot cause
mem_saverecords activity under the concrete observation session ID, whilemem_searchandmem_contextevaluate nudges underdefaultSessionID(project). A fresh observation could therefore be returned together withNo mem_save calls for this project ....Updating the synthetic default session would make the warning disappear, but it breaks the existing explicit-session ActivityScore contract. This change keeps project freshness as separate state instead.
Related history: #449 improved active-session routing for saves, but does not address project-level read nudges.
Related nudge issue: #668 covers client-hook timezone and first-observation behavior, not this server-side identity mismatch.
Tracked by #703. This PR remains draft until that issue receives
status:approved;Closes #703will be added after approval.Test plan
Notes
The full
internal/mcpsuite in an anonymous checkout has unrelated existing project-detection fixture failures because the checkout's detected project is not backed in test stores. The targeted affected suite is green, and the fix received two review passes with no remaining Critical or Important findings.