Skip to content

fix(mcp): skip conflict candidates for session summaries - #1023

Merged
dnlrsls merged 2 commits into
Gentleman-Programming:mainfrom
dnlrsls:fix/mem-session-summary-conflict
Sep 5, 2026
Merged

dnlrsls merged 2 commits into
Gentleman-Programming:mainfrom
dnlrsls:fix/mem-session-summary-conflict

Conversation

@dnlrsls

@dnlrsls dnlrsls commented Sep 5, 2026

Copy link
Copy Markdown
Member

🔗 Linked Issue

Closes #1016


🏷️ PR Type

  • type:bug — Bug fix
  • type:feature — New feature
  • type:docs — Documentation only
  • type:refactor — Code refactoring (no behavior change)
  • type:chore — Maintenance, dependencies, tooling
  • type:breaking-change — Breaking change

📝 Summary

  • Stop session summaries from running semantic conflict-candidate discovery after persistence.
  • Preserve persisted summary metadata while returning judgment_required: false without pending adjudications.

📂 Changes

File Change
internal/mcp/mcp.go Remove session-summary candidate discovery and always return no adjudication requirement.
internal/mcp/mcp_test.go Replace candidate-surfacing coverage with regression coverage for no candidate metadata or pending relations.

🧪 Test Plan

  • Focused regression test passed: go test ./internal/mcp -count=1 -run '^(TestHandleSessionSummarySkipsConflictCandidates|TestHandleSave_CandidatesReturned|TestHandleSave_NoCandidates_ResultUnchanged)$'
  • Unit tests pass locally: go test ./... (not run locally; CI pending)
  • E2E tests pass locally: go test -tags e2e ./internal/server/... (not run locally; CI pending)
  • Manually tested the affected functionality (not performed; focused automated coverage passed)

🤖 Automated Checks

These run automatically and all must pass before merge:

Check What it verifies Status
Check Issue Reference PR body contains Closes #N / Fixes #N / Resolves #N
Check Issue Has status:approved Linked issue has status:approved label
Check PR Has type:* Label PR has exactly one type:* label
Unit Tests go test ./... passes
E2E Tests go test -tags e2e ./internal/server/... passes
Plugin Tests npm test passes in plugin/pi

✅ Contributor Checklist

  • I linked an approved issue above (Closes #N)
  • I added exactly one type:* label to this PR
  • I ran unit tests locally: go test ./...
  • I ran e2e tests locally: go test -tags e2e ./internal/server/...
  • Docs updated (if behavior changed)
  • Commits follow conventional commits format
  • No Co-Authored-By trailers in commits

💬 Notes for Reviewers

PR #679 independently adds session-summary author attribution in the same function. If it merges after this PR, preserve both that author-attribution behavior and this PR's no-adjudication behavior; resolve the shared hunk accordingly.

Summary by CodeRabbit

  • Behavior Changes
    • Session summaries no longer include conflict-candidate metadata.
    • Summary responses now consistently report no judgment required while retaining saved summary details.
    • Multiple session summaries continue to be stored without leaving pending relationship records.

@dnlrsls dnlrsls added the type:bug Bug fix label Sep 5, 2026
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Team

Run ID: 1f0f3b52-48ba-4da3-aa42-03335ebf7856

📥 Commits

Reviewing files that changed from the base of the PR and between 7ef2a6d and d88b8b7.

📒 Files selected for processing (2)
  • internal/mcp/mcp.go
  • internal/mcp/mcp_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

Changes

The session-summary flow no longer detects conflict candidates or returns judgment metadata. Tests now verify direct persistence, absent candidate fields, distinct summaries, and zero pending memory relations.

Session summary flow

Layer / File(s) Summary
Direct session summary response
internal/mcp/mcp.go
handleSessionSummary builds the response from the saved observation and sets judgment_required=false.
Candidate metadata removal tests
internal/mcp/mcp_test.go
Session-summary tests verify that candidate metadata is absent, summaries persist distinctly, and no pending relations remain.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 0597c

Session summaries now save and return without conflict adjudication metadata or pending judgment relations, while retaining their persisted summary fields. No concrete merge-blocking risk remains.

Suggested reviewers: gentleman-programming, alan-thegentleman, cinlodev

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR implements the primary behavior requested in [#1016] by skipping candidate detection, returning judgment_required=false, preserving summary metadata, and testing that no pending relations remai… Add the required bounded timing evidence for queue_wait_ms, store_ms, candidate-scan, and total duration, or update the linked issue if that requirement is no longer applicable. Preserve the current behavior that session summaries do not cr…
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: session summaries skip conflict-candidate discovery.
Out of Scope Changes check ✅ Passed The changes remain within the stated scope of [#1016]. They remove session-summary candidate detection and update related regression tests. No unrelated code changes are described.
Full details: Linked Issues check

Explanation

The PR implements the primary behavior requested in [#1016] by skipping candidate detection, returning judgment_required=false, preserving summary metadata, and testing that no pending relations remain. However, the linked issue also requires bounded queue_wait_ms, store_ms, candidate-scan, and total timing evidence, which is not present in the described changes.

Resolution

Add the required bounded timing evidence for queue_wait_ms, store_ms, candidate-scan, and total duration, or update the linked issue if that requirement is no longer applicable. Preserve the current behavior that session summaries do not create conflict adjudications, while keeping normal mem_save conflict handling unchanged.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dnlrsls
dnlrsls merged commit 1b2ec65 into Gentleman-Programming:main Sep 5, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(mcp): mem_session_summary blocks session close on semantic conflict adjudication

1 participant