Skip to content

Commit 17e3710

Browse files
BenjaminMichaelisCopilot
BenjaminMichaelis
and
Copilot
committed
fix: apply multi-agent code review findings to MCP tools
- Remove Destructive = false from all 17 MCP tool annotations (default per spec) - Fix FindBookHelpForDiagnostic description to accurately reflect guidelines output - Add 500-char max length validation to SearchBookContent, LookupConcept, CheckTopicCoverage, FindBookHelpForDiagnostic - Add missing empty check to SearchBookContent - Make GetSectionContent async (File.ReadAllTextAsync + doc.LoadHtml, removes TOCTOU File.Exists) - Add path canonicalization via Path.GetRelativePath (robust cross-platform, handles drive-root edge case) - Add .html file extension allowlist check on resolved path - Add AnchorId validation via [GeneratedRegex] before XPath interpolation - Separate IOException catches: FileNotFoundException/DirectoryNotFoundException show 'not generated yet', UnauthorizedAccessException and IOException show generic messages (no ex.Message leak) - Make BookContentTool a partial class for GeneratedRegex support Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 61de3df commit 17e3710

5 files changed

Lines changed: 219 additions & 105 deletions

File tree

‎EssentialCSharp.Web/Tools/BookContentTool.cs‎

Lines changed: 81 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System.ComponentModel;
22
using System.Globalization;
33
using System.Text;
4+
using System.Text.RegularExpressions;
45
using EssentialCSharp.Chat.Common.Services;
56
using EssentialCSharp.Web.Extensions;
67
using EssentialCSharp.Web.Services;
@@ -10,7 +11,7 @@
1011
namespace EssentialCSharp.Web.Tools;
1112

1213
[McpServerToolType]
13-
public sealed class BookContentTool
14+
public sealed partial class BookContentTool
1415
{
1516
private readonly ISiteMappingService _siteMappingService;
1617
private readonly IListingSourceCodeService _listingService;
@@ -32,32 +33,72 @@ public BookContentTool(
3233
_searchService = serviceProvider.GetService<AISearchService>();
3334
}
3435

35-
[McpServerTool(Title = "Get Section Content", ReadOnly = true, Destructive = false, Idempotent = true, OpenWorld = false),
36+
[McpServerTool(Title = "Get Section Content", ReadOnly = true, Idempotent = true, OpenWorld = false),
3637
Description("Retrieve the prose content of a specific book section identified by its slug/key (e.g., 'hello-world', 'creating-editing-compiling-and-running-c-source-code'). Returns the section text with code examples preserved. Use GetChapterSections to discover available slugs.")]
37-
public string GetSectionContent(
38+
public async Task<string> GetSectionContent(
3839
[Description("The section slug/key (e.g., 'hello-world'). Use GetChapterSections to get valid slugs.")] string sectionKey,
39-
[Description("Maximum number of characters to return (500–8000, default 4000). Long sections are truncated.")] int maxChars = 4000)
40+
[Description("Maximum number of characters to return (500–8000, default 4000). Long sections are truncated.")] int maxChars = 4000,
41+
CancellationToken cancellationToken = default)
4042
{
4143
maxChars = Math.Clamp(maxChars, 500, 8000);
4244

45+
if (string.IsNullOrWhiteSpace(sectionKey))
46+
{
47+
return "Section key must not be empty. Use GetChapterSections to discover valid section slugs.";
48+
}
49+
4350
SiteMapping? mapping = _siteMappingService.SiteMappings.Find(sectionKey);
4451
if (mapping is null)
4552
{
4653
return $"Section '{sectionKey}' not found. Use GetChapterSections to discover valid section slugs.";
4754
}
48-
if (mapping.AnchorId is null)
55+
if (mapping.AnchorId is null || string.IsNullOrWhiteSpace(mapping.AnchorId))
4956
{
5057
return $"Section '{sectionKey}' does not have an anchor ID and cannot be extracted.";
5158
}
59+
if (!AnchorIdRegex().IsMatch(mapping.AnchorId))
60+
{
61+
return $"Section '{sectionKey}' has an invalid anchor ID.";
62+
}
5263

53-
string filePath = Path.Join(_environment.ContentRootPath, Path.Join(mapping.PagePath));
54-
if (!File.Exists(filePath))
64+
string contentRoot = Path.GetFullPath(_environment.ContentRootPath);
65+
string filePath = Path.GetFullPath(Path.Join(contentRoot, Path.Join(mapping.PagePath)));
66+
string relative = Path.GetRelativePath(contentRoot, filePath);
67+
if (relative == ".." ||
68+
relative.StartsWith(".." + Path.DirectorySeparatorChar, StringComparison.Ordinal) ||
69+
Path.IsPathRooted(relative))
70+
{
71+
return $"Section '{sectionKey}' has an invalid path.";
72+
}
73+
if (!string.Equals(Path.GetExtension(filePath), ".html", StringComparison.OrdinalIgnoreCase))
74+
{
75+
return $"Section '{sectionKey}' has an invalid path.";
76+
}
77+
78+
string htmlContent;
79+
try
80+
{
81+
htmlContent = await File.ReadAllTextAsync(filePath, cancellationToken);
82+
}
83+
catch (FileNotFoundException)
5584
{
5685
return $"Chapter HTML file not found for section '{sectionKey}'. Content may not be generated yet.";
5786
}
87+
catch (DirectoryNotFoundException)
88+
{
89+
return $"Chapter HTML file not found for section '{sectionKey}'. Content may not be generated yet.";
90+
}
91+
catch (UnauthorizedAccessException)
92+
{
93+
return $"Chapter HTML could not be accessed for section '{sectionKey}'.";
94+
}
95+
catch (IOException)
96+
{
97+
return $"Failed to read chapter HTML for section '{sectionKey}'.";
98+
}
5899

59100
HtmlDocument doc = new();
60-
doc.Load(filePath);
101+
doc.LoadHtml(htmlContent);
61102

62103
var sectionNode = doc.DocumentNode.SelectSingleNode(
63104
$"//div[@id='{mapping.AnchorId}' and contains(@class,'section-heading')]");
@@ -68,11 +109,12 @@ public string GetSectionContent(
68109
}
69110

70111
var parent = sectionNode.ParentNode;
71-
var sb = new StringBuilder();
72-
sb.AppendLine(CultureInfo.InvariantCulture, $"## {mapping.RawHeading}");
73-
sb.AppendLine(CultureInfo.InvariantCulture, $"Chapter {mapping.ChapterNumber}: {mapping.ChapterTitle}");
74-
sb.AppendLine();
112+
var header = new StringBuilder();
113+
header.AppendLine(CultureInfo.InvariantCulture, $"## {mapping.RawHeading}");
114+
header.AppendLine(CultureInfo.InvariantCulture, $"Chapter {mapping.ChapterNumber}: {mapping.ChapterTitle}");
115+
header.AppendLine();
75116

117+
var body = new StringBuilder();
76118
bool collecting = false;
77119
foreach (HtmlNode child in parent.ChildNodes)
78120
{
@@ -91,19 +133,19 @@ public string GetSectionContent(
91133
break;
92134
}
93135

94-
ExtractNodeContent(child, sb);
136+
ExtractNodeContent(child, body);
95137

96-
if (sb.Length >= maxChars)
138+
if (body.Length >= maxChars)
97139
{
98-
sb.Append("\n\n[Content truncated — use a larger maxChars value to see more.]");
140+
body.Append("\n\n[Content truncated — use a larger maxChars value to see more.]");
99141
break;
100142
}
101143
}
102144

103-
return sb.Length == 0 ? $"No content found after section heading '{mapping.RawHeading}'." : sb.ToString();
145+
return body.Length == 0 ? $"No content found after section heading '{mapping.RawHeading}'." : header.Append(body).ToString();
104146
}
105147

106-
[McpServerTool(Title = "Get Listing With Context", ReadOnly = true, Destructive = false, Idempotent = true, OpenWorld = false),
148+
[McpServerTool(Title = "Get Listing With Context", ReadOnly = true, Idempotent = true, OpenWorld = false),
107149
Description("Retrieve a specific book listing's source code together with the semantic book content that explains it. Combines code from GetListingSourceCode with related explanatory text found via search. Ideal for understanding what a listing demonstrates.")]
108150
public async Task<string> GetListingWithContext(
109151
[Description("The chapter number of the listing.")] int chapter,
@@ -116,7 +158,7 @@ public async Task<string> GetListingWithContext(
116158
return $"Listing {chapter}.{listing} not found. Verify the chapter and listing numbers.";
117159
}
118160

119-
string langHint = NormalizeExtension(response.FileExtension);
161+
string langHint = BookToolHelpers.NormalizeExtension(response.FileExtension);
120162
var sb = new StringBuilder();
121163
sb.AppendLine(CultureInfo.InvariantCulture, $"## Listing {response.ChapterNumber}.{response.ListingNumber}");
122164
sb.AppendLine(CultureInfo.InvariantCulture, $"```{langHint}");
@@ -149,11 +191,16 @@ public async Task<string> GetListingWithContext(
149191
return sb.ToString();
150192
}
151193

152-
[McpServerTool(Title = "Get Navigation Context", ReadOnly = true, Destructive = false, Idempotent = true, OpenWorld = false),
194+
[McpServerTool(Title = "Get Navigation Context", ReadOnly = true, Idempotent = true, OpenWorld = false),
153195
Description("Get the navigation context for a book section: its breadcrumb path, the previous and next sections, its parent section, and its sibling sections. Useful for understanding where a section sits in the book's structure.")]
154196
public string GetNavigationContext(
155197
[Description("The section slug/key (e.g., 'hello-world'). Use GetChapterSections to get valid slugs.")] string sectionKey)
156198
{
199+
if (string.IsNullOrWhiteSpace(sectionKey))
200+
{
201+
return "Section key must not be empty. Use GetChapterSections to discover valid section slugs.";
202+
}
203+
157204
SiteMapping? mapping = _siteMappingService.SiteMappings.Find(sectionKey);
158205
if (mapping is null)
159206
{
@@ -166,7 +213,7 @@ public string GetNavigationContext(
166213
.ThenBy(m => m.OrderOnPage)
167214
.ToList();
168215

169-
int idx = ordered.IndexOf(mapping);
216+
int idx = ordered.FindIndex(m => ReferenceEquals(m, mapping));
170217
if (idx < 0)
171218
{
172219
return $"Section '{sectionKey}' could not be located in the ordered mapping list.";
@@ -213,7 +260,7 @@ public string GetNavigationContext(
213260
}
214261
if (parent is not null)
215262
{
216-
sb.AppendLine(CultureInfo.InvariantCulture, $"**Parent:** {parent.RawHeading} (`/{parent.Keys.First()}#{parent.AnchorId}`)");
263+
sb.AppendLine(CultureInfo.InvariantCulture, $"**Parent:** {parent.RawHeading} (`/{parent.Keys.FirstOrDefault() ?? parent.PrimaryKey}#{parent.AnchorId}`)");
217264
sb.AppendLine();
218265
}
219266

@@ -230,7 +277,7 @@ public string GetNavigationContext(
230277
}
231278
if (prev is not null)
232279
{
233-
sb.AppendLine(CultureInfo.InvariantCulture, $"**Previous:** {prev.RawHeading} (`/{prev.Keys.First()}#{prev.AnchorId}`)");
280+
sb.AppendLine(CultureInfo.InvariantCulture, $"**Previous:** {prev.RawHeading} (`/{prev.Keys.FirstOrDefault() ?? prev.PrimaryKey}#{prev.AnchorId}`)");
234281
}
235282

236283
// Next section at same indent level in the same chapter
@@ -247,13 +294,13 @@ public string GetNavigationContext(
247294
}
248295
if (next is not null)
249296
{
250-
sb.AppendLine(CultureInfo.InvariantCulture, $"**Next:** {next.RawHeading} (`/{next.Keys.First()}#{next.AnchorId}`)");
297+
sb.AppendLine(CultureInfo.InvariantCulture, $"**Next:** {next.RawHeading} (`/{next.Keys.FirstOrDefault() ?? next.PrimaryKey}#{next.AnchorId}`)");
251298
}
252299

253300
// Siblings: all siblings sharing the same parent
254301
if (parent is not null)
255302
{
256-
int parentIdx = ordered.IndexOf(parent);
303+
int parentIdx = ordered.FindIndex(m => ReferenceEquals(m, parent));
257304
var siblings = new List<SiteMapping>();
258305
for (int i = parentIdx + 1; i < ordered.Count; i++)
259306
{
@@ -270,15 +317,15 @@ public string GetNavigationContext(
270317
sb.AppendLine("**Sibling sections:**");
271318
foreach (var s in siblings)
272319
{
273-
sb.AppendLine(CultureInfo.InvariantCulture, $" - {s.RawHeading} (`/{s.Keys.First()}#{s.AnchorId}`)");
320+
sb.AppendLine(CultureInfo.InvariantCulture, $" - {s.RawHeading} (`/{s.Keys.FirstOrDefault() ?? s.PrimaryKey}#{s.AnchorId}`)");
274321
}
275322
}
276323
}
277324

278325
return sb.ToString();
279326
}
280327

281-
[McpServerTool(Title = "Get Chapter Summary", ReadOnly = true, Destructive = false, Idempotent = true, OpenWorld = false),
328+
[McpServerTool(Title = "Get Chapter Summary", ReadOnly = true, Idempotent = true, OpenWorld = false),
282329
Description("Get a structural overview of a book chapter: its top-level section headings in reading order, and the coding guidelines associated with that chapter. Useful for understanding what a chapter covers before diving in.")]
283330
public string GetChapterSummary(
284331
[Description("The chapter number (e.g., 5 for Chapter 5).")] int chapter)
@@ -304,7 +351,7 @@ public string GetChapterSummary(
304351
foreach (var m in chapterMappings.Where(m => m.IndentLevel <= 1))
305352
{
306353
string indent = m.IndentLevel == 0 ? "" : " ";
307-
string link = $"`/{m.Keys.First()}#{m.AnchorId}`";
354+
string link = $"`/{m.Keys.FirstOrDefault() ?? m.PrimaryKey}#{m.AnchorId}`";
308355
sb.AppendLine(CultureInfo.InvariantCulture, $"{indent}- {m.RawHeading} ({link})");
309356
}
310357

@@ -356,10 +403,11 @@ private static void ExtractNodeContent(HtmlNode node, StringBuilder sb)
356403
{
357404
foreach (var line in codeLines)
358405
{
359-
// Remove the line-number span before extracting text
360-
var lineNumberSpan = line.SelectSingleNode(".//span[contains(@class,'code-line-number')]");
406+
// Clone to avoid mutating the live HtmlDocument DOM
407+
var lineClone = line.CloneNode(deep: true);
408+
var lineNumberSpan = lineClone.SelectSingleNode(".//span[contains(@class,'code-line-number')]");
361409
lineNumberSpan?.Remove();
362-
sb.AppendLine(HtmlEntity.DeEntitize(line.InnerText));
410+
sb.AppendLine(HtmlEntity.DeEntitize(lineClone.InnerText));
363411
}
364412
}
365413
sb.AppendLine("```");
@@ -385,16 +433,6 @@ private static void ExtractNodeContent(HtmlNode node, StringBuilder sb)
385433
}
386434
}
387435

388-
private static string NormalizeExtension(string ext) =>
389-
ext.TrimStart('.').ToLowerInvariant() switch
390-
{
391-
"cs" => "csharp",
392-
"vb" => "vbnet",
393-
"fs" => "fsharp",
394-
"" => "",
395-
var e => e
396-
};
397-
398436
private static string FormatGuidelineType(GuidelineType type) => type switch
399437
{
400438
GuidelineType.Do => "DO",
@@ -403,4 +441,7 @@ private static string NormalizeExtension(string ext) =>
403441
GuidelineType.DoNot => "DO NOT",
404442
_ => "NOTE"
405443
};
444+
445+
[GeneratedRegex(@"^[A-Za-z0-9_-]{1,128}$")]
446+
private static partial Regex AnchorIdRegex();
406447
}

‎EssentialCSharp.Web/Tools/BookGuidelinesTool.cs‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ public BookGuidelinesTool(IGuidelinesService guidelinesService)
1616
_guidelinesService = guidelinesService;
1717
}
1818

19-
[McpServerTool(Title = "Get C# Guidelines", ReadOnly = true, Destructive = false, Idempotent = true, OpenWorld = false),
19+
[McpServerTool(Title = "Get C# Guidelines", ReadOnly = true, Idempotent = true, OpenWorld = false),
2020
Description("Retrieve C# coding guidelines from the Essential C# book. Optionally filter by keyword, chapter number, or guideline type (do/consider/avoid/donot). The book contains guidelines covering naming conventions, error handling, LINQ, async/await, generics, and many other topics. Each guideline includes its chapter and subsection context.")]
2121
public string GetCSharpGuidelines(
2222
[Description("Optional keyword to filter guidelines by (searched in guideline text and subsection name).")] string? keyword = null,
@@ -27,6 +27,11 @@ public string GetCSharpGuidelines(
2727
maxResults = Math.Clamp(maxResults, 1, 50);
2828
GuidelineType? typeFilter = ParseGuidelineType(type);
2929

30+
if (type is not null && typeFilter is null)
31+
{
32+
return "Invalid guideline type. Valid values: 'do', 'consider', 'avoid', 'donot' (also accepts 'do not', 'dont').";
33+
}
34+
3035
IEnumerable<GuidelineListing> filtered = _guidelinesService.Guidelines;
3136

3237
if (chapter.HasValue)
@@ -62,7 +67,7 @@ public string GetCSharpGuidelines(
6267
return sb.ToString();
6368
}
6469

65-
[McpServerTool(Title = "Get Guidelines By Topic", ReadOnly = true, Destructive = false, Idempotent = true, OpenWorld = false),
70+
[McpServerTool(Title = "Get Guidelines By Topic", ReadOnly = true, Idempotent = true, OpenWorld = false),
6671
Description("Search C# coding guidelines from the Essential C# book by topic or concept. More discoverable than filtering by chapter — finds all guidelines related to exceptions, naming, async, LINQ, generics, interfaces, and more. Results are ordered by relevance to the topic.")]
6772
public string GetGuidelinesByTopic(
6873
[Description("The topic or concept to search guidelines for (e.g., 'exception handling', 'naming', 'async', 'LINQ', 'generics', 'interface').")] string topic,
@@ -112,7 +117,7 @@ public string GetGuidelinesByTopic(
112117
{
113118
if (string.IsNullOrWhiteSpace(input)) return null;
114119

115-
return input.Trim().ToLowerInvariant().Replace(" ", "").Replace("_", "") switch
120+
return input.Trim().ToLowerInvariant().Replace(" ", "").Replace("_", "").Replace("'", "") switch
116121
{
117122
"do" => GuidelineType.Do,
118123
"consider" => GuidelineType.Consider,

‎EssentialCSharp.Web/Tools/BookListingTool.cs‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ public BookListingTool(IListingSourceCodeService listingService, ISiteMappingSer
1818
_siteMappingService = siteMappingService;
1919
}
2020

21-
[McpServerTool(Title = "Get Listing Source Code", ReadOnly = true, Destructive = false, Idempotent = true, OpenWorld = false),
21+
[McpServerTool(Title = "Get Listing Source Code", ReadOnly = true, Idempotent = true, OpenWorld = false),
2222
Description("Retrieve the complete source code for a specific numbered listing from the Essential C# book. Example: chapter=5, listing=3 retrieves Listing 5.3. Returns the code and its file type.")]
2323
public async Task<string> GetListingSourceCode(
2424
[Description("The chapter number containing the listing (e.g., 5 for Chapter 5).")] int chapter,
@@ -31,11 +31,11 @@ public async Task<string> GetListingSourceCode(
3131
return $"Listing {chapter}.{listing} not found. Verify that both the chapter and listing numbers are correct.";
3232
}
3333

34-
string langHint = NormalizeExtension(response.FileExtension);
34+
string langHint = BookToolHelpers.NormalizeExtension(response.FileExtension);
3535
return $"## Listing {response.ChapterNumber}.{response.ListingNumber}\n\n```{langHint}\n{response.Content}\n```";
3636
}
3737

38-
[McpServerTool(Title = "Search Listings By Code", ReadOnly = true, Destructive = false, Idempotent = true, OpenWorld = false),
38+
[McpServerTool(Title = "Search Listings By Code", ReadOnly = true, Idempotent = true, OpenWorld = false),
3939
Description("Search all code listings in the Essential C# book for a specific code pattern, keyword, or identifier. Searches actual C# source code (not prose). Useful for finding examples of Task.WhenAll, yield return, IDisposable, pattern matching, and similar code constructs.")]
4040
public async Task<string> SearchListingsByCode(
4141
[Description("The code pattern or keyword to search for in listing source code (case-insensitive substring match).")] string pattern,
@@ -68,7 +68,7 @@ public async Task<string> SearchListingsByCode(
6868
if (found >= maxResults) break;
6969
if (listing.Content.Contains(pattern, StringComparison.OrdinalIgnoreCase))
7070
{
71-
string langHint = NormalizeExtension(listing.FileExtension);
71+
string langHint = BookToolHelpers.NormalizeExtension(listing.FileExtension);
7272
sb.AppendLine(CultureInfo.InvariantCulture, $"### Listing {listing.ChapterNumber}.{listing.ListingNumber}");
7373
sb.AppendLine(CultureInfo.InvariantCulture, $"```{langHint}");
7474
sb.AppendLine(listing.Content);
@@ -87,14 +87,4 @@ public async Task<string> SearchListingsByCode(
8787
sb.Insert(0, $"# Listings Containing '{pattern}' ({found} result{(found == 1 ? "" : "s")})\n\n");
8888
return sb.ToString();
8989
}
90-
91-
private static string NormalizeExtension(string ext) =>
92-
ext.TrimStart('.').ToLowerInvariant() switch
93-
{
94-
"cs" => "csharp",
95-
"vb" => "vbnet",
96-
"fs" => "fsharp",
97-
"" => "",
98-
var e => e
99-
};
10090
}

0 commit comments

Comments
 (0)