fix(mcp): shrink serverInstructions CONFLICT SURFACING block - #627
fix(mcp): shrink serverInstructions CONFLICT SURFACING block#627ardelperal wants to merge 3 commits into
Conversation
…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".
…#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.
📝 WalkthroughWalkthroughThe MCP conflict-surfacing instructions now use concise decision rules. A test verifies that ChangesMCP server instructions
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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/issue1019_length_test.go`:
- Around line 21-28: Extend TestServerInstructions_StaysUnder2048CharsIssue1019
to assert that serverInstructions contains each of the ten required routing
phrases, using deterministic checks alongside the existing rune-limit assertion.
Preserve the current boundary validation and report missing phrases clearly so
changes to the instruction text cannot remove required routing keywords without
failing the test.
🪄 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: d490e71f-a03a-4111-af94-d12fa9de35c5
📒 Files selected for processing (2)
internal/mcp/issue1019_length_test.gointernal/mcp/mcp.go
| func TestServerInstructions_StaysUnder2048CharsIssue1019(t *testing.T) { | ||
| const claudeCodeTruncationCeiling = 2048 | ||
| runes := utf8.RuneCountInString(serverInstructions) | ||
| t.Logf("serverInstructions rune count: %d (byte count: %d)", runes, len(serverInstructions)) | ||
| if runes >= claudeCodeTruncationCeiling { | ||
| t.Errorf("serverInstructions is %d runes (>=%d) — Claude Code will truncate and SIGINT the connection per issue #1019. Trim prose.", | ||
| runes, claudeCodeTruncationCeiling) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Also assert the required routing phrases.
This test only verifies the rune limit, so it will still pass if the conflict-surfacing rewrite removes or changes any of the ten required keywords. Add deterministic assertions for those phrases alongside the boundary check. As per path instructions, tests must cover happy paths, 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/mcp/issue1019_length_test.go` around lines 21 - 28, Extend
TestServerInstructions_StaysUnder2048CharsIssue1019 to assert that
serverInstructions contains each of the ten required routing phrases, using
deterministic checks alongside the existing rune-limit assertion. Preserve the
current boundary validation and report missing phrases clearly so changes to the
instruction text cannot remove required routing keywords without failing the
test.
Source: Path instructions
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
The premise checks out — I measured it. serverInstructions on main is 2539 runes / 2565 bytes against a documented 2048 truncation limit, and your branch brings it to 1400. The compressed block still contains all 10 strings TestServerInstructions_ConflictSurfacingBlock requires, so that test keeps passing. This is worth having.
Blocking items:
-
Closes #1019doesn't resolve here. That issue lives inGentleman-Programming/gentle-ai. The PR body asserts "GitHub links the PR against the issue regardless of repo" — that isn't true; cross-repoCloses #Ndoes not link, and theCheck Issue Referencegate can't pass. Please file an issue in this repo, getstatus:approved, and retarget. - Drop the issue number from permanent source.
issue1019_length_test.go/TestServerInstructions_StaysUnder2048CharsIssue1019hardcode a number that doesn't exist in this repo. Suggestserverinstructions_length_test.goandTestServerInstructionsStaysUnderClientTruncationLimit. - Soften the causal claim. "Client SIGINTs the server ~4s after handshake because of truncation" is asserted from one log correlation, not reproduced. State the measured fact (truncation at 2048 runes) and leave the SIGINT link out unless you can reproduce it.
- Rewrite the body from the template — the current encoding is corrupted (
## ?? Linked Issue). Test plan also citesgo test ./cmd/engramfor a test that lives ininternal/mcp.
type:bug applied.
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.
|
Hey @Alan-TheGentleman — addressed all four items from your CHANGES_REQUESTED on 2026-07-20. Ready for re-review. Item-by-item:
Pending maintainer action (single item, documented in PR body
Once #700 has Force-pushed HEAD: |
2 participants
🔗 Linked Issue
Closes #700
🏷️ PR Type
type:bug- Bug fixtype:feature- New featuretype:docs- Documentation onlytype:refactor- Code refactoring (no behavior change)type:chore- Maintenance, dependencies, toolingtype:breaking-change- Breaking change📝 Summary
Fixes a class of MCP-handshake regressions against Claude Code where
mem_*tools stopped responding ~2-5 seconds after connect.serverInstructions(returned during MCP handshake) measures 2539 runes, exceeding the documented 2048-rune truncation ceiling of the Claude Code MCP client.## CONFLICT SURFACINGblock ofserverInstructionsis compressed from ~30 lines to ~6, preserving all 10 routing keywords that the pre-existingTestServerInstructions_ConflictSurfacingBlockvalidates. Final size 1400 runes, with ~640 runes of margin vs the cap.TestServerInstructionsStaysUnderClientTruncationLimit(ininternal/mcp/serverinstructions_length_test.go) fails if the constant ever crosses the cap again.The chain from truncation to SIGINT/timing is documented in field reports but not reproduced in this repo — the assertion in the test comment has been softened to reflect that.
📂 Changes
internal/mcp/mcp.go## CONFLICT SURFACINGblock ofserverInstructionscompressed from ~30 lines to ~6 dense lines. All 10 routing keywords (judgment_required,candidates[],mem_judge,0.7,supersedes,conflicts_with,architecture,conversationally,evidence, plus the section header) preserved inline soTestServerInstructions_ConflictSurfacingBlockstill passes. Other sections ofserverInstructionsunchanged.internal/mcp/serverinstructions_length_test.go(new, +34 lines)TestServerInstructionsStaysUnderClientTruncationLimit— measuresutf8.RuneCountInString(serverInstructions)and fails with a clear message referencing the client truncation ceiling. Tripwire for future regressions.Diffstat vs
origin/main:🧪 Test Plan
Tests run during development
TestServerInstructionsConstantIsNonEmptyTestServerInstructions_ConflictSurfacingBlock(pre-existing — verifies all 10 required phrases)TestServerInstructionsStaysUnderClientTruncationLimit(new — verifies rune count < 2048)Runtime output:
Quality gates
go vet ./internal/mcp/...→ cleangofmt -l internal/mcp→ only pre-existing CRLF drift (matches repo convention)No E2E run locally
The
go test -tags e2e ./internal/server/...suite requires Docker, not available in this contributor environment. CI will run it.🤖 Automated Checks
Closes #700status:approvedstatus:approvedfrom maintainer (see Pending maintainer actions)type:*labeltype:bugappliedgo test ./...passesgo test -tags e2e ./internal/server/...passes✅ Contributor Checklist
Closes #700)type:*label to this PR (type:bug)./internal/mcp)Co-Authored-Bytrailers in commits💬 Notes for Reviewers
Root cause verification
The numbers in the fix match the empirical evidence to the rune:
[debug] Server instructions truncated from 2539 to 2048 charsutf8.RuneCountInString(serverInstructions) == 2539(2565 bytes)utf8.RuneCountInString(serverInstructions) == 1400(well below 2048)The truncation-only mismatch is the entire bug.
What the tripwire does NOT assert
The pre-rename version of this test asserted "Claude Code SIGINTs the connection ~4s after handshake when truncation happens". That claim is sourced from a single log correlation in the field reports, not reproduced in this repo. The new test comment explicitly marks the SIGINT/timing chain as field-report evidence, not a verified property of Claude Code. If a future contributor wants to harden the causal claim, it must come with a reproduction, not a rephrasing.
Why this fix doesn't touch the regression-introducing commit (
889dae7)889dae7 feat(memory): add memory-conflict-surfacing (Phase 1)(Alan, 26-Apr-2026) is the commit that grewserverInstructionsfrom ~430 chars to 2539. We do NOT revert or rewrite that commit's prose. We compress the affected block to its essential semantics (one dense paragraph that preserves all 10 routing keywords) so the original commit's intent is honored at smaller byte cost. The pre-existingTestServerInstructions_ConflictSurfacingBlockcontinues to pass.Why no E2E run
The MCP-server handshake regression is reproducible end-to-end only with a real Claude Code client on the network. Per
go vetplus the focused unit tests, all in-scope signals are green. CI will run E2E.Pending maintainer actions
The following are maintainer-applied per
pr-check.ymlof the upstream org's convention — not within contributor scope on this fork:status:approvedto issue fix(mcp): serverInstructions constant exceeds Claude Code's 2048-rune truncation limit #700 soCheck Issue Has status:approvedpasses on this PRNo
size:exceptionrequested (engram has no 400-line gate; PR is34 +/29 -over 2 files).