Recover complete object from concatenated JSON in sampling responses#53
Merged
Conversation
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>
There was a problem hiding this comment.
Pull request overview
This PR hardens SamplingHelper.SampleStructuredAsync<T> against a sampling-backend variant where a truncated false-start JSON object and the complete object are concatenated within a single content block, causing direct JSON deserialization to fail.
Changes:
- Adds a
TryDeserialize<T>fallback that scans for candidate{starts and selects the deserializable object that consumes the most bytes (recovering the complete object when multiple objects are concatenated). - Increases failure-log truncation for raw sampling blocks from
500to2000characters to make malformed responses more visible. - Adds regression tests covering “single block, two objects” and “valid JSON followed by trailing prose”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| PrCopilot/src/PrCopilot/Tools/SamplingHelper.cs | Adds fallback object-extraction logic for concatenated JSON and increases debug-log truncation size. |
| PrCopilot/tests/PrCopilot.Tests/SamplingHelperTests.cs | Adds tests for concatenated-objects-in-one-block and trailing-prose parsing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #52. Fixes another variant of the sampling failure that surfaces as
Sampling failed to explain comment after 2 attempts/'e' is invalid after a value(Path: $).The sampling backend can return a truncated false-start object and the complete object concatenated in a single content block (
blocks=1):#52 handled the case where these arrive as separate content blocks (
blocks=2) by trying each block. But when both objects are in one block, per-block selection has nothing to split, so a direct parse still fails (the first unterminated string swallows up to the second object's key-quote, landing on theeof the second"explanation").Changes
TryDeserialize<T>: on direct-parse failure, fall back toTryExtractBestObject<T>, which scans each{, deserializes one object viaUtf8JsonReader(ignoring trailing content), and keeps the object that consumes the most bytes. The truncated false-start fails to parse; the complete object wins.500->2000so future malformations are fully visible in the debug log (the 500 cap hid this one).Tests
SingleBlockTwoObjects_UsesCompleteObject— the exactblocks=1two-object failure.TrailingProseAfterObject_StillParses— bonus: a complete object followed by trailing prose.All 352 tests pass; both new tests verified to fail against the previous code. Composes with #52, covering
blocks=1andblocks=2. Validated live on a dev build before opening.