Add source_url to MCP get_document_by_url output#3476
Conversation
The MCP get_document_by_url tool now returns a source_url field containing the GitHub blob URL for the source file of each documentation page. The URL is computed at indexing time using the same git context that powers the existing "edit this page" link in the frontend, and stored as a keyword field in the Elasticsearch index. API reference docs (sourced from the temporary OpenAPI exporter) will have a null source_url until the planned Elastic.ApiExplorer pipeline is in place. Tests cover the mapping path with a stub gateway and validate the URL format against a live index.
|
Label error. Requires exactly 1 of: automation, breaking, bug, changelog:skip, chore, ci, dependencies, documentation, enhancement, feature, fix, redesign. Found: |
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. 📝 WalkthroughWalkthroughThis PR adds a 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
✨ Simplify code
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: 2
🤖 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 `@src/api/Elastic.Documentation.Mcp.Remote/Responses/McpResponses.cs`:
- Line 113: The SourceUrl property in McpResponses.cs currently serializes to
"sourceUrl" but the MCP contract requires "source_url"; update the SourceUrl
property to explicitly specify the JSON name (e.g., add
[JsonPropertyName("source_url")] above the public string? SourceUrl { get; init;
}) so the serializer emits "source_url" (ensure the System.Text.Json attribute
is imported or use [JsonProperty("source_url")] if the project uses
Newtonsoft.Json).
In `@tests/Mcp.Remote.Tests/DocumentToolsTests.cs`:
- Around line 31-33: The tests currently deserialize JSON into DocumentResponse
(via JsonSerializer.Deserialize with McpJsonContext.Default.DocumentResponse)
and assert response.SourceUrl, which won't detect if the emitted JSON property
name changed; update the tests to also assert the raw JSON string contains the
expected key "source_url" (or that "source_url" is absent when expected null)
before/after deserialization so a field-name contract regression is
caught—locate the assertions around response/DocumentResponse (lines with
JsonSerializer.Deserialize and response!.SourceUrl) and add a string-based
assertion against the raw json variable for the "source_url" property (and
mirror the same change for the other test at the noted lines 50-52).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3820dcc8-d841-4e4b-9dc1-5af3f1b4a013
📒 Files selected for processing (8)
src/Elastic.Markdown/Exporters/Elasticsearch/ElasticsearchMarkdownExporter.Export.cssrc/api/Elastic.Documentation.Mcp.Remote/Gateways/DocumentGateway.cssrc/api/Elastic.Documentation.Mcp.Remote/Gateways/IDocumentGateway.cssrc/api/Elastic.Documentation.Mcp.Remote/Responses/McpResponses.cssrc/api/Elastic.Documentation.Mcp.Remote/Tools/DocumentTools.cssrc/services/search/Elastic.Documentation.Search.Contract/DocumentationDocument.cstests-integration/Mcp.Remote.IntegrationTests/DocumentToolsIntegrationTests.cstests/Mcp.Remote.Tests/DocumentToolsTests.cs
| public string[]? AiQuestions { get; init; } | ||
| public string[]? AiUseCases { get; init; } | ||
| public DateTimeOffset? LastUpdated { get; init; } | ||
| public string? SourceUrl { get; init; } |
There was a problem hiding this comment.
SourceUrl currently serializes as sourceUrl, not source_url (contract mismatch).
Line 113 needs an explicit JSON name override if the MCP contract is source_url as stated in the PR objective. With current options, this will emit sourceUrl and can break clients expecting source_url.
Proposed fix
public sealed record DocumentResponse
{
@@
+ [JsonPropertyName("source_url")]
public string? SourceUrl { get; init; }
@@
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public string? SourceUrl { get; init; } | |
| [JsonPropertyName("source_url")] | |
| public string? SourceUrl { get; init; } |
🤖 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 `@src/api/Elastic.Documentation.Mcp.Remote/Responses/McpResponses.cs` at line
113, The SourceUrl property in McpResponses.cs currently serializes to
"sourceUrl" but the MCP contract requires "source_url"; update the SourceUrl
property to explicitly specify the JSON name (e.g., add
[JsonPropertyName("source_url")] above the public string? SourceUrl { get; init;
}) so the serializer emits "source_url" (ensure the System.Text.Json attribute
is imported or use [JsonProperty("source_url")] if the project uses
Newtonsoft.Json).
| var response = JsonSerializer.Deserialize(json, McpJsonContext.Default.DocumentResponse); | ||
| response.Should().NotBeNull(); | ||
| response!.SourceUrl.Should().Be(expectedSourceUrl); |
There was a problem hiding this comment.
These tests won’t catch a JSON field-name contract regression.
Because both assertions deserialize into DocumentResponse, they validate value mapping but not the emitted key name. Add a raw JSON assertion for "source_url" (or explicit absence when null) so contract breaks are caught.
Also applies to: 50-52
🤖 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 `@tests/Mcp.Remote.Tests/DocumentToolsTests.cs` around lines 31 - 33, The tests
currently deserialize JSON into DocumentResponse (via JsonSerializer.Deserialize
with McpJsonContext.Default.DocumentResponse) and assert response.SourceUrl,
which won't detect if the emitted JSON property name changed; update the tests
to also assert the raw JSON string contains the expected key "source_url" (or
that "source_url" is absent when expected null) before/after deserialization so
a field-name contract regression is caught—locate the assertions around
response/DocumentResponse (lines with JsonSerializer.Deserialize and
response!.SourceUrl) and add a string-based assertion against the raw json
variable for the "source_url" property (and mirror the same change for the other
test at the noted lines 50-52).
1 participant
The MCP get_document_by_url tool now returns a source_url field containing
the GitHub blob URL for the source file of each documentation page. The URL
is computed at indexing time using the same git context that powers the
existing "edit this page" link in the frontend, and stored as a keyword
field in the Elasticsearch index. API reference docs (sourced from the
temporary OpenAPI exporter) will have a null source_url until the planned
Elastic.ApiExplorer pipeline is in place. Tests cover the mapping path with
a stub gateway and validate the URL format against a live index.