Skip to content

Conversation

@sgammon
Copy link
Member

@sgammon sgammon commented Jul 13, 2025

Ready for review Powered by Pull Request Badge

Summary

MCP configuration introduced a bug because of a clash with the type parameter to server blocks.

Signed-off-by: Sam Gammon <sam@elide.dev>
@sgammon sgammon self-assigned this Jul 13, 2025
@sgammon sgammon added bug Something isn't working module:cli CLI module issues and features labels Jul 13, 2025
@sgammon sgammon requested a review from Copilot July 13, 2025 17:56
@sgammon sgammon added this to the Release R18: Beta milestone Jul 13, 2025
@sgammon sgammon changed the title fix: crash with elide init w.r.t. mcp config fix(cli): crash with elide init w.r.t. mcp config Jul 13, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a crash when running elide init by resolving a naming clash in the MCP configuration and hardening request handling.

  • Added a null check for sessionId query parameter in the MCP server endpoint
  • Disabled JSON class discriminators to avoid collisions with a reserved type field
  • Improved text normalization logic in AgentAdvice to collapse excess newlines via regex

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
packages/tooling/src/main/kotlin/elide/tooling/project/mcp/ModelContextProtocol.kt Add null check for missing sessionId query parameter to prevent NPE
packages/tooling/src/main/kotlin/elide/tooling/project/mcp/McpProjectConfig.kt Opt in to ExperimentalSerializationApi and set classDiscriminatorMode = NONE
packages/tooling/src/main/kotlin/elide/tooling/project/agents/AgentAdvice.kt Replace manual newline replace with a Regex("\n{2,}") call for better trimming logic
Comments suppressed due to low confidence (1)

packages/tooling/src/main/kotlin/elide/tooling/project/mcp/McpProjectConfig.kt:47

  • [nitpick] Add a brief comment explaining why ClassDiscriminatorMode.NONE is used here to clarify the intent and avoid confusion for future maintainers.
        classDiscriminatorMode = ClassDiscriminatorMode.NONE

@codecov
Copy link

codecov bot commented Jul 13, 2025

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 40.43%. Comparing base (f257ba8) to head (c012663).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
.../elide/tooling/project/mcp/ModelContextProtocol.kt 0.00% 4 Missing ⚠️
...tlin/elide/tooling/project/mcp/McpProjectConfig.kt 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1534      +/-   ##
==========================================
- Coverage   40.45%   40.43%   -0.02%     
==========================================
  Files         749      749              
  Lines       35325    35329       +4     
  Branches     4967     4968       +1     
==========================================
- Hits        14290    14287       -3     
- Misses      19332    19340       +8     
+ Partials     1703     1702       -1     
Flag Coverage Δ
jvm 40.43% <16.66%> (-0.02%) ⬇️
lib 40.43% <16.66%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...kotlin/elide/tooling/project/agents/AgentAdvice.kt 41.07% <100.00%> (ø)
...tlin/elide/tooling/project/mcp/McpProjectConfig.kt 0.00% <0.00%> (ø)
.../elide/tooling/project/mcp/ModelContextProtocol.kt 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f257ba8...c012663. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sgammon sgammon merged commit 77b234d into main Jul 13, 2025
18 checks passed
@elidebot elidebot mentioned this pull request Jul 19, 2025
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working module:cli CLI module issues and features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants