Skip to content

fix(mcp): preserve compact conflict judgment contract - #800

Open
dnlrsls wants to merge 4 commits into
Gentleman-Programming:mainfrom
dnlrsls:fix/pr-627-judgment-id-contract
Open

fix(mcp): preserve compact conflict judgment contract#800
dnlrsls wants to merge 4 commits into
Gentleman-Programming:mainfrom
dnlrsls:fix/pr-627-judgment-id-contract

Conversation

@dnlrsls

@dnlrsls dnlrsls commented Aug 26, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

🔗 Linked Issue

Closes #700

🏷️ PR Type

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

📝 Summary

  • Carries forward the serverInstructions compression and 2048-rune regression guard from fix(mcp): shrink serverInstructions CONFLICT SURFACING block #627.
  • Restores the load-bearing contract that each candidates[] entry uses its own judgment_id and the top-level ID is never reused.
  • Adds focused semantic coverage so future compression cannot silently remove that distinction.

📂 Changes

File Change
internal/mcp/mcp.go Keep conflict-surfacing instructions compact while restoring the per-candidate judgment ID rule.
internal/mcp/serverinstructions_length_test.go Enforce the rune ceiling and the restored semantic contract.

🧪 Test Plan

  • Focused MCP tests pass: go test ./internal/mcp -count=1
  • Full local suite: go test ./... exceeded the 10-minute local limit without reporting a failure; CI is required.
  • E2E tests were not run locally; CI remains the merge gate.
  • Final serverInstructions size is 1464 runes, leaving 584 below the 2048-rune repository guard.

🤖 Automated Checks

Check What it verifies Status
Check Issue Reference PR body contains Closes #700 ⏳
Check Issue Has status:approved Issue #700 is approved ⏳
Check PR Has type: Label* Exactly one type label ⏳
Unit Tests go test ./... ⏳
E2E Tests Repository E2E suite ⏳

✅ Contributor Checklist

💬 Notes for Reviewers

This maintainer PR supersedes #627 because triage credentials could not update the contributor fork directly. The original implementation and attribution remain with @ardelperal; the dropped-contract finding was independently reported by @yudelsuarez.

The bounded correction commit is 3f7fe83189f817de83fb7de1e42aa1a9d2e5fb87. Its exact two-file candidate completed native RDD approval, focused MCP tests pass, and no merge is requested until CI is green.

Summary by CodeRabbit

  • Bug Fixes

    • Improved conflict guidance to ensure each candidate is evaluated with its own judgment ID.
    • Clarified handling of user questions and supported conflict-resolution relationships.
    • Reduced instruction length to prevent truncation in supported MCP clients.

  • Tests

    • Added coverage verifying instruction length and candidate judgment ID requirements.

ardelperal and others added 4 commits July 19, 2026 10:28
@ardelperal
test(mcp): guard serverInstructions under 2048-rune MCP client cap (i…
97a46bb 
…ssue #1019)

Claude Code's MCP client truncates server instructions at 2048 runes
during handshake and SIGINTs the connection when truncation occurs.
This test asserts the boundary so any future prose addition that
pushes the constant over the cap is caught locally before CI.

Rune count is measured with utf8.RuneCountInString; em-dashes,
arrows, and other multibyte runes each decode as one rune, so the
plain byte len() of the string misleadingly understates the figure
Claude Code sees. Issue #1019 reported the failure as
"Server instructions truncated from 2539 to 2048 chars".
@ardelperal
fix(mcp): shrink serverInstructions CONFLICT SURFACING block (closes …
61271ec 
…#1019)

The CONFLICT SURFACING prose ballooned to 2539 runes, pushing the
constant past Claude Code's 2048-rune MCP truncation cap and breaking
the handshake in the plugin marketplace path. The shorter form
keeps all ten required key phrases (judgment_required, candidates[],
mem_judge, 0.7, supersedes, conflicts_with, architecture,
conversationally, evidence, ## CONFLICT SURFACING) so
TestServerInstructions_ConflictSurfacingBlock still passes.

Rune count drops from 2539 to 1400. No other section of
serverInstructions was modified.
@ardelperal
test(mcp): rename length tripwire and soften causal claim
16d5bdb 
The tripwire test was named after issue #1019 (which lives in the
Gentleman-Programming/gentle-ai orchestrator repo), making it
cross-repo-coupled. A new engram-native issue will carry the fix;
the tripwire is renamed to match.

The test comment previously asserted a causal chain (truncation causes
the MCP client to SIGINT ~4s after handshake) sourced from a single
log correlation. Soften the comment to state the measured fact
(2048-rune truncation ceiling) and explicitly mark the SIGINT/timing
chain as field-report evidence not reproduced in this repo.

No production code change; internal/mcp/mcp.go untouched.
@dnlrsls
fix(mcp): preserve per-candidate judgment IDs
3f7fe83
@dnlrsls dnlrsls added the type:bug Bug fix label Aug 26, 2026
@coderabbitai

coderabbitai Bot commented Aug 26, 2026 •
edited
Loading

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: Pro Plus

Run ID: 0af08673-bf3b-4559-a992-805243246dce

📥 Commits

Reviewing files that changed from the base of the PR and between 59cb8f2 and 3f7fe83.

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

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

📝 Walkthrough

Walkthrough

The MCP server’s conflict-surfacing instructions were shortened. New tests enforce the 2048-rune limit and verify candidate-specific judgment_id guidance.

Changes

MCP instruction compaction

Layer / File(s) Summary
Condense and validate server instructions
internal/mcp/mcp.go, internal/mcp/serverinstructions_length_test.go		 The conflict-surfacing instructions now use a shorter format. Tests enforce the rune limit and verify candidate-specific judgment_id usage.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 3f7fe

This localized change preserves compact conflict instructions while ensuring each candidate retains its own judgment ID; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

Suggested reviewers: gentleman-programming, alan-thegentleman

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the MCP change: preserving a compact conflict judgment contract.
Linked Issues check ✅ Passed The changes address issue #700 by reducing serverInstructions below the 2048-rune limit, adding a permanent rune-count tripwire, preserving per-candidate judgment IDs and conflict-surfacing semantics,…
Out of Scope Changes check ✅ Passed The changes are limited to compressing MCP server instructions and adding focused regression tests. They directly support the requirements in issue #700.
Full details: Linked Issues check

Explanation

The changes address issue #700 by reducing serverInstructions below the 2048-rune limit, adding a permanent rune-count tripwire, preserving per-candidate judgment IDs and conflict-surfacing semantics, and leaving mem_* behavior unchanged.

  • Fix all pre-merge checks with AI
✨ 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.

Alan-TheGentleman
Alan-TheGentleman reviewed Aug 27, 2026
View reviewed changes

@Alan-TheGentleman Alan-TheGentleman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source-level fix for #700 is sound and preserves candidate-specific judgment guidance. The exact head is behind main, so this is not ready for approval. Please rebase onto current main and rerun the full CI suite on the rebased head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Alan-TheGentleman Alan-TheGentleman Alan-TheGentleman left review comments

Assignees

No one assigned

Labels

type:bug Bug fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(mcp): serverInstructions constant exceeds Claude Code's 2048-rune truncation limit

3 participants

@dnlrsls @Alan-TheGentleman @ardelperal