From b4106a6a387fefcd67148e4dad301ab7dd557ece Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Thu, 2 Jul 2026 11:32:37 -0700 Subject: [PATCH 1/9] Recover complete object from concatenated JSON in sampling responses The sampling backend can return a truncated false-start object and the complete object concatenated in a single content block, which a direct parse rejects. On parse failure, scan for the largest parseable top-level object (via Utf8JsonReader, ignoring trailing content) and use it. Also bump the failure-log truncation to 2000 chars for diagnosability. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/PrCopilot/Tools/SamplingHelper.cs | 67 +++++++++++++++++-- .../PrCopilot.Tests/SamplingHelperTests.cs | 33 +++++++++ 2 files changed, 93 insertions(+), 7 deletions(-) diff --git a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs index b5c056c..a03c279 100644 --- a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs +++ b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs @@ -91,7 +91,7 @@ internal static class SamplingHelper } DebugLogger.Log("Sampling", $"Failed to parse JSON response as {typeof(T).Name} from {blocks.Count} block(s): {lastError}"); - DebugLogger.Log("Sampling", $"Raw blocks: {string.Join(" || ", blocks).Truncate(500)}"); + DebugLogger.Log("Sampling", $"Raw blocks: {string.Join(" || ", blocks).Truncate(2000)}"); return null; } @@ -104,26 +104,79 @@ private static bool TryDeserialize(string text, out T? parsed, out string? er { parsed = null; error = null; + + // Strip markdown code fences if present (LLMs often wrap JSON in ```json ... ```) + // and escape literal control chars inside strings (invalid per RFC 8259). + var cleaned = SanitizeJsonControlChars(StripCodeFences(text)); + try { - // Strip markdown code fences if present (LLMs often wrap JSON in ```json ... ```) - // and escape literal control chars inside strings (invalid per RFC 8259). - var cleaned = SanitizeJsonControlChars(StripCodeFences(text)); parsed = JsonSerializer.Deserialize(cleaned, _jsonOptions); - return parsed != null; + if (parsed != null) + return true; } catch (JsonException ex) { error = ex.Message; - return false; } catch (FileNotFoundException ex) { // .NET single-file publishing can throw FileNotFoundException instead of JsonException // when satellite assemblies for JSON error messages are missing. error = $"[{ex.GetType().Name}] {ex.Message}"; - return false; } + + // Fallback: the text may contain a truncated false-start object followed by the + // complete object, both in one block (e.g. {"a": "trunc\n{"a": "complete"}). + // A direct parse fails, so extract the largest parseable top-level object instead. + if (TryExtractBestObject(cleaned, out parsed)) + { + error = null; + return true; + } + + return false; + } + + /// + /// Scan the text for top-level JSON objects and return the one that parses into + /// while consuming the most bytes. This recovers the complete + /// object when the response also contains a truncated false-start object that a direct + /// parse chokes on. + /// + private static bool TryExtractBestObject(string text, out T? parsed) where T : class + { + parsed = null; + long bestConsumed = -1; + + for (int i = 0; i < text.Length; i++) + { + if (text[i] != '{') + continue; + + try + { + var bytes = Encoding.UTF8.GetBytes(text[i..]); + var reader = new Utf8JsonReader(bytes); + var candidate = JsonSerializer.Deserialize(ref reader, _jsonOptions); + + // Deserialize(ref reader) reads exactly one value and ignores anything + // after it, so trailing content (a second object, prose, etc.) is fine. + if (candidate != null && reader.BytesConsumed > bestConsumed) + { + bestConsumed = reader.BytesConsumed; + parsed = candidate; + } + } + catch (JsonException) + { + } + catch (FileNotFoundException) + { + } + } + + return parsed != null; } /// diff --git a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs index 4e3b6e5..cd3aaca 100644 --- a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs +++ b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs @@ -399,6 +399,39 @@ public async Task SampleStructuredAsync_MultipleBlocks_OneObjectSplitAcrossBlock Assert.Equal("clarify", result.RecommendationType); } + [Fact] + public async Task SampleStructuredAsync_SingleBlockTwoObjects_UsesCompleteObject() + { + // The exact PR 16255 failure: a truncated false-start object and the complete + // object arrive concatenated in ONE content block (blocks=1). + var truncated = "{\"explanation\": \"The reviewer notes that deleting the analyzer means AZC0005 is no longer enfor"; + var complete = "{\"explanation\": \"The reviewer flags missing test coverage.\", \"recommendation\": \"Push back; the removal is intentional and in scope.\", \"recommendationType\": \"pushback\"}"; + var server = new FakeSamplingMcpServer(truncated + "\n" + complete); + + var result = await SamplingHelper.SampleStructuredAsync( + server, "system", "user", maxTokens: 100); + + Assert.NotNull(result); + Assert.Equal("The reviewer flags missing test coverage.", result!.Explanation); + Assert.Contains("intentional", result.Recommendation); + Assert.Equal("pushback", result.RecommendationType); + } + + [Fact] + public async Task SampleStructuredAsync_TrailingProseAfterObject_StillParses() + { + // Complete object followed by trailing prose the model tacked on. + var response = "{\"explanation\": \"done\", \"recommendation\": \"fix\", \"recommendationType\": \"implement\"}\n\nLet me know if you'd like more detail."; + var server = new FakeSamplingMcpServer(response); + + var result = await SamplingHelper.SampleStructuredAsync( + server, "system", "user", maxTokens: 100); + + Assert.NotNull(result); + Assert.Equal("done", result!.Explanation); + Assert.Equal("implement", result.RecommendationType); + } + private class TestResponse { public string? Name { get; set; } From 7812d20767b58d7c04748ed9481e3937fd8c0824 Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Thu, 2 Jul 2026 12:50:05 -0700 Subject: [PATCH 2/9] Fix spellcheck: add 'parseable' to cspell, reword artificial fragments Add the legitimate term 'parseable' to cspell.json and replace the placeholder word fragments 'trunc'/'enfor' in a comment example and a test string with real words, so cspell strict mode passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PrCopilot/cspell.json | 1 + PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs | 2 +- PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/PrCopilot/cspell.json b/PrCopilot/cspell.json index b9abb15..4b637dd 100644 --- a/PrCopilot/cspell.json +++ b/PrCopilot/cspell.json @@ -14,6 +14,7 @@ "nologo", "nums", "osascript", + "parseable", "pushback", "slnx", "unreplied", diff --git a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs index a03c279..22f7c48 100644 --- a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs +++ b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs @@ -127,7 +127,7 @@ private static bool TryDeserialize(string text, out T? parsed, out string? er } // Fallback: the text may contain a truncated false-start object followed by the - // complete object, both in one block (e.g. {"a": "trunc\n{"a": "complete"}). + // complete object, both in one block (e.g. {"a": "part\n{"a": "whole"}). // A direct parse fails, so extract the largest parseable top-level object instead. if (TryExtractBestObject(cleaned, out parsed)) { diff --git a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs index cd3aaca..777f737 100644 --- a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs +++ b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs @@ -404,7 +404,7 @@ public async Task SampleStructuredAsync_SingleBlockTwoObjects_UsesCompleteObject { // The exact PR 16255 failure: a truncated false-start object and the complete // object arrive concatenated in ONE content block (blocks=1). - var truncated = "{\"explanation\": \"The reviewer notes that deleting the analyzer means AZC0005 is no longer enfor"; + var truncated = "{\"explanation\": \"The reviewer notes that deleting the analyzer means AZC0005 is no longer enforced"; var complete = "{\"explanation\": \"The reviewer flags missing test coverage.\", \"recommendation\": \"Push back; the removal is intentional and in scope.\", \"recommendationType\": \"pushback\"}"; var server = new FakeSamplingMcpServer(truncated + "\n" + complete); From 431a925f7e676c479cfe773668078055b0694375 Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Thu, 2 Jul 2026 12:56:38 -0700 Subject: [PATCH 3/9] Align TryExtractBestObject doc with its scan-every-brace behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs index 22f7c48..fb0ce2d 100644 --- a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs +++ b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs @@ -139,10 +139,11 @@ private static bool TryDeserialize(string text, out T? parsed, out string? er } /// - /// Scan the text for top-level JSON objects and return the one that parses into + /// Scan the text for candidate object starts (every '{', which may include nested or + /// non-top-level positions) and return the value that deserializes into /// while consuming the most bytes. This recovers the complete /// object when the response also contains a truncated false-start object that a direct - /// parse chokes on. + /// parse chokes on; candidate starts that aren't a valid object simply fail to parse. /// private static bool TryExtractBestObject(string text, out T? parsed) where T : class { From 83ba115f5c776cf4245057226dbc65f8dfe45cc8 Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Thu, 2 Jul 2026 13:18:36 -0700 Subject: [PATCH 4/9] Short-circuit TryExtractBestObject and document its catch blocks Break once a candidate consumes the entire remaining suffix (no later start can consume more), avoiding O(n^2) allocations on brace-heavy text, and explain why the catch blocks intentionally skip invalid candidate starts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs index fb0ce2d..ef71f04 100644 --- a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs +++ b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs @@ -167,13 +167,22 @@ private static bool TryExtractBestObject(string text, out T? parsed) where T { bestConsumed = reader.BytesConsumed; parsed = candidate; + + // If this object consumed the whole remaining suffix, no later start + // (which has fewer bytes to work with) can beat it — stop early. + if (reader.BytesConsumed == bytes.Length) + break; } } catch (JsonException) { + // Not a valid object start (e.g. a '{' inside a string, or a truncated + // false-start) — expected while scanning; try the next candidate. } catch (FileNotFoundException) { + // Single-file publishing can surface JSON parse errors as this instead; + // treat the same as an invalid candidate. } } From f91e1ebf908f9bae224dee1558909d7138f595f9 Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Thu, 2 Jul 2026 13:32:32 -0700 Subject: [PATCH 5/9] Drop inaccurate 'top-level' from TryDeserialize fallback comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs index ef71f04..ff64299 100644 --- a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs +++ b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs @@ -128,7 +128,7 @@ private static bool TryDeserialize(string text, out T? parsed, out string? er // Fallback: the text may contain a truncated false-start object followed by the // complete object, both in one block (e.g. {"a": "part\n{"a": "whole"}). - // A direct parse fails, so extract the largest parseable top-level object instead. + // A direct parse fails, so extract the largest parseable object instead. if (TryExtractBestObject(cleaned, out parsed)) { error = null; From cbf51bfd3132e4227740e6a749f20b1394ea01ba Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Mon, 6 Jul 2026 11:28:38 -0700 Subject: [PATCH 6/9] Bound raw-blocks log allocation with a capped join Replace string.Join(...).Truncate(...) (which materializes the full joined string first) with JoinCapped, which stops appending at the cap so the log allocation stays bounded even when a content block is very large. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/PrCopilot/Tools/SamplingHelper.cs | 32 ++++++++++++++++++- .../PrCopilot.Tests/SamplingHelperTests.cs | 20 ++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs index ff64299..20499e7 100644 --- a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs +++ b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs @@ -91,10 +91,40 @@ internal static class SamplingHelper } DebugLogger.Log("Sampling", $"Failed to parse JSON response as {typeof(T).Name} from {blocks.Count} block(s): {lastError}"); - DebugLogger.Log("Sampling", $"Raw blocks: {string.Join(" || ", blocks).Truncate(2000)}"); + DebugLogger.Log("Sampling", $"Raw blocks: {JoinCapped(blocks, " || ", 2000)}"); return null; } + /// + /// Join with , appending at most + /// characters so the result allocation stays bounded even + /// when individual parts are very large (used for diagnostic logging). + /// + internal static string JoinCapped(IReadOnlyList parts, string separator, int maxLength) + { + var sb = new StringBuilder(Math.Min(maxLength + 3, 1024)); + foreach (var part in parts) + { + if (sb.Length >= maxLength) + break; + if (sb.Length > 0) + sb.Append(separator); + + var remaining = maxLength - sb.Length; + if (part.Length <= remaining) + { + sb.Append(part); + } + else + { + sb.Append(part, 0, remaining).Append("..."); + break; + } + } + + return sb.ToString(); + } + /// /// Attempt to deserialize a single candidate response into , /// stripping code fences and escaping stray control characters first. diff --git a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs index 777f737..f9c01e4 100644 --- a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs +++ b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs @@ -432,6 +432,26 @@ public async Task SampleStructuredAsync_TrailingProseAfterObject_StillParses() Assert.Equal("implement", result.RecommendationType); } + [Fact] + public void JoinCapped_ShortInput_JoinsFully() + { + var parts = new List { "one", "two", "three" }; + var result = SamplingHelper.JoinCapped(parts, " || ", 2000); + Assert.Equal("one || two || three", result); + } + + [Fact] + public void JoinCapped_HugeInput_BoundsAllocation() + { + var parts = new List { new string('a', 5000), new string('b', 5000) }; + var result = SamplingHelper.JoinCapped(parts, " || ", 2000); + + // Never materializes the full 10k+ join — bounded to the cap plus the "..." marker. + Assert.True(result.Length <= 2003, $"length was {result.Length}"); + Assert.EndsWith("...", result); + Assert.StartsWith("aaa", result); + } + private class TestResponse { public string? Name { get; set; } From cef65b9e220e05c30a34c4ce42f5ea039a514b27 Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Mon, 6 Jul 2026 13:45:49 -0700 Subject: [PATCH 7/9] Fix JoinCapped negative-remaining crash on near-cap separator Only append the separator when the next part still fits under maxLength, so the separator can't push sb.Length past the cap and drive remaining negative (which threw ArgumentOutOfRangeException in the failure-logging path). Add a regression test for the first-part-leaves-<-separator case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs | 11 +++++++++-- .../tests/PrCopilot.Tests/SamplingHelperTests.cs | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs index 20499e7..a9dac23 100644 --- a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs +++ b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs @@ -102,12 +102,19 @@ internal static class SamplingHelper /// internal static string JoinCapped(IReadOnlyList parts, string separator, int maxLength) { + if (maxLength <= 0) + return string.Empty; + var sb = new StringBuilder(Math.Min(maxLength + 3, 1024)); foreach (var part in parts) { - if (sb.Length >= maxLength) + // Only append the separator if the following part has room — otherwise the + // separator alone could push past maxLength and make remaining negative. + var separatorLength = sb.Length > 0 ? separator.Length : 0; + if (sb.Length + separatorLength >= maxLength) break; - if (sb.Length > 0) + + if (separatorLength > 0) sb.Append(separator); var remaining = maxLength - sb.Length; diff --git a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs index f9c01e4..feff6b6 100644 --- a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs +++ b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs @@ -452,6 +452,21 @@ public void JoinCapped_HugeInput_BoundsAllocation() Assert.StartsWith("aaa", result); } + [Fact] + public void JoinCapped_FirstPartLeavesLessThanSeparator_DoesNotThrow() + { + // Regression: the first part leaves fewer than separator-length chars before the cap + // (1999 chars, 2000 cap, 4-char separator). Previously the separator was appended + // unconditionally, making remaining negative and throwing ArgumentOutOfRangeException + // in the failure-logging path. + var parts = new List { new string('a', 1999), new string('b', 100) }; + var result = SamplingHelper.JoinCapped(parts, " || ", 2000); + + Assert.True(result.Length <= 2003, $"length was {result.Length}"); + Assert.StartsWith("aaa", result); + Assert.DoesNotContain("b", result); + } + private class TestResponse { public string? Name { get; set; } From 0904aaa16f6498c1558ae73d85bd14fad38ee7a5 Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Mon, 6 Jul 2026 13:56:49 -0700 Subject: [PATCH 8/9] Clarify JoinCapped doc: output may be maxLength + 3 with '...' marker Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs index a9dac23..d3f50ff 100644 --- a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs +++ b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs @@ -97,8 +97,10 @@ internal static class SamplingHelper /// /// Join with , appending at most - /// characters so the result allocation stays bounded even - /// when individual parts are very large (used for diagnostic logging). + /// characters of content (plus a trailing "..." marker when + /// truncated, so the result is at most + 3 characters). This + /// keeps the allocation bounded even when individual parts are very large (used for + /// diagnostic logging). /// internal static string JoinCapped(IReadOnlyList parts, string separator, int maxLength) { From 00c9b93c32d994fe7b9bb4f4cd22d02b524205ee Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Mon, 6 Jul 2026 15:55:26 -0700 Subject: [PATCH 9/9] Append truncation marker when JoinCapped drops parts mid-join The separator-doesn't-fit break exited without the '...' marker, so a truncated diagnostic log looked complete. Append the marker before breaking, and add tests asserting it appears on between-parts truncation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs | 4 ++++ .../tests/PrCopilot.Tests/SamplingHelperTests.cs | 14 ++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs index d3f50ff..fc3c355 100644 --- a/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs +++ b/PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs @@ -114,7 +114,11 @@ internal static string JoinCapped(IReadOnlyList parts, string separator, // separator alone could push past maxLength and make remaining negative. var separatorLength = sb.Length > 0 ? separator.Length : 0; if (sb.Length + separatorLength >= maxLength) + { + // No room for this part — content is being dropped, so mark the truncation. + sb.Append("..."); break; + } if (separatorLength > 0) sb.Append(separator); diff --git a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs index feff6b6..57d78db 100644 --- a/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs +++ b/PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs @@ -465,6 +465,20 @@ public void JoinCapped_FirstPartLeavesLessThanSeparator_DoesNotThrow() Assert.True(result.Length <= 2003, $"length was {result.Length}"); Assert.StartsWith("aaa", result); Assert.DoesNotContain("b", result); + // Content was dropped between parts — the truncation marker must be present. + Assert.EndsWith("...", result); + } + + [Fact] + public void JoinCapped_TruncatesBetweenParts_AppendsMarker() + { + // The second part doesn't fit even the separator, so it's dropped between parts. + var parts = new List { new string('a', 8), new string('b', 8) }; + var result = SamplingHelper.JoinCapped(parts, " || ", 10); + + Assert.StartsWith("aaa", result); + Assert.DoesNotContain("b", result); + Assert.EndsWith("...", result); } private class TestResponse