mirror of
https://github.com/davidkaya/aryx.git
synced 2026-07-26 14:38:45 +02:00
fix: remove streaming text boundary heuristics that injected spurious spaces
The StreamingTextMerger (C#) and mergeStreamingText (TypeScript) contained custom word-boundary heuristics that inserted spaces between streaming deltas based on incorrect assumptions. Two bugs caused mid-word spaces: 1. Unconditional space insertion when incoming delta started with uppercase or digit characters, breaking acronyms (MDA -> M DA, UAG -> UA G) 2. LooksLikeWordBoundary using global token counts that fired on nearly every mid-word token split (writable -> wr itable, fragile -> frag ile) Both upstream projects (Copilot SDK and Agent Framework) use simple string concatenation for delta accumulation. LLM streaming tokens natively include whitespace when needed, so no boundary detection is required. Removed all boundary heuristic methods and replaced with plain concatenation. Kept overlap detection and snapshot handling for event redelivery scenarios. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -1,5 +1,3 @@
|
||||
using System.Text.RegularExpressions;
|
||||
|
||||
namespace Aryx.AgentHost.Services;
|
||||
|
||||
internal static partial class StreamingTextMerger
|
||||
@@ -7,7 +5,6 @@ internal static partial class StreamingTextMerger
|
||||
private const double SnapshotReplacementMinLengthRatio = 0.6;
|
||||
private const int SnapshotReplacementMinTokenCount = 3;
|
||||
private const double SnapshotReplacementSharedTokenRatio = 0.5;
|
||||
private const string CharactersThatDoNotNeedLeadingSpace = "([{/\"'`";
|
||||
|
||||
public static string Merge(string current, string incoming)
|
||||
{
|
||||
@@ -32,51 +29,7 @@ internal static partial class StreamingTextMerger
|
||||
return incoming;
|
||||
}
|
||||
|
||||
return current + ResolveBoundarySeparator(current, incoming) + incoming;
|
||||
}
|
||||
|
||||
private static bool TryMergeSnapshotVariants(string current, string incoming, out string merged)
|
||||
{
|
||||
if (incoming.StartsWith(current, StringComparison.Ordinal)
|
||||
|| incoming.Contains(current, StringComparison.Ordinal))
|
||||
{
|
||||
merged = incoming;
|
||||
return true;
|
||||
}
|
||||
|
||||
if (current.Contains(incoming, StringComparison.Ordinal))
|
||||
{
|
||||
merged = current;
|
||||
return true;
|
||||
}
|
||||
|
||||
merged = string.Empty;
|
||||
return false;
|
||||
}
|
||||
|
||||
private static bool TryMergeByOverlap(string current, string incoming, out string merged)
|
||||
{
|
||||
int overlapLength = ComputeSuffixPrefixOverlap(current, incoming);
|
||||
if (overlapLength == 0)
|
||||
{
|
||||
merged = string.Empty;
|
||||
return false;
|
||||
}
|
||||
|
||||
merged = current + incoming[overlapLength..];
|
||||
return true;
|
||||
}
|
||||
|
||||
private static string ResolveBoundarySeparator(string current, string incoming)
|
||||
{
|
||||
if (ShouldInsertNewlineBoundary(current, incoming))
|
||||
{
|
||||
return "\n";
|
||||
}
|
||||
|
||||
return ShouldInsertSpaceBoundary(current, incoming)
|
||||
? " "
|
||||
: string.Empty;
|
||||
return current + incoming;
|
||||
}
|
||||
|
||||
private static int ComputeSuffixPrefixOverlap(string current, string incoming)
|
||||
@@ -125,54 +78,6 @@ internal static partial class StreamingTextMerger
|
||||
&& incomingTokens.Count >= SnapshotReplacementMinTokenCount;
|
||||
}
|
||||
|
||||
private static bool ShouldInsertNewlineBoundary(string current, string incoming)
|
||||
{
|
||||
return !current.EndsWith('\n')
|
||||
&& MarkdownBlockPrefixRegex().IsMatch(incoming.TrimStart());
|
||||
}
|
||||
|
||||
private static bool ShouldInsertSpaceBoundary(string current, string incoming)
|
||||
{
|
||||
char lastCharacter = current[^1];
|
||||
char firstCharacter = incoming[0];
|
||||
if (HasExistingBoundary(lastCharacter, firstCharacter)
|
||||
|| CharactersThatDoNotNeedLeadingSpace.Contains(lastCharacter))
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
if (ClosingPunctuationRegex().IsMatch(incoming))
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
return StartsLikeASeparatedInlineFragment(firstCharacter, incoming)
|
||||
|| LooksLikeWordBoundary(current, incoming);
|
||||
}
|
||||
|
||||
private static bool HasExistingBoundary(char lastCharacter, char firstCharacter)
|
||||
{
|
||||
return char.IsWhiteSpace(lastCharacter) || char.IsWhiteSpace(firstCharacter);
|
||||
}
|
||||
|
||||
private static bool StartsLikeASeparatedInlineFragment(char firstCharacter, string incoming)
|
||||
{
|
||||
return MarkdownInlinePrefixRegex().IsMatch(incoming)
|
||||
|| char.IsUpper(firstCharacter)
|
||||
|| char.IsDigit(firstCharacter);
|
||||
}
|
||||
|
||||
private static bool LooksLikeWordBoundary(string current, string incoming)
|
||||
{
|
||||
string[] currentTokens = Tokenize(current).ToArray();
|
||||
string[] incomingTokens = Tokenize(incoming).ToArray();
|
||||
string firstIncomingToken = incomingTokens.FirstOrDefault() ?? string.Empty;
|
||||
|
||||
return currentTokens.Length >= 2
|
||||
&& incomingTokens.Length >= 2
|
||||
&& firstIncomingToken.Length >= 2;
|
||||
}
|
||||
|
||||
private static IEnumerable<string> Tokenize(string value)
|
||||
{
|
||||
return TokenRegex()
|
||||
@@ -181,15 +86,38 @@ internal static partial class StreamingTextMerger
|
||||
.Where(token => token.Length > 0);
|
||||
}
|
||||
|
||||
[GeneratedRegex("[a-z0-9]+", RegexOptions.IgnoreCase)]
|
||||
private static partial Regex TokenRegex();
|
||||
private static bool TryMergeSnapshotVariants(string current, string incoming, out string merged)
|
||||
{
|
||||
if (incoming.StartsWith(current, StringComparison.Ordinal)
|
||||
|| incoming.Contains(current, StringComparison.Ordinal))
|
||||
{
|
||||
merged = incoming;
|
||||
return true;
|
||||
}
|
||||
|
||||
[GeneratedRegex(@"^[.,!?;:%)\]}]")]
|
||||
private static partial Regex ClosingPunctuationRegex();
|
||||
if (current.Contains(incoming, StringComparison.Ordinal))
|
||||
{
|
||||
merged = current;
|
||||
return true;
|
||||
}
|
||||
|
||||
[GeneratedRegex(@"^[*_`~\[]")]
|
||||
private static partial Regex MarkdownInlinePrefixRegex();
|
||||
merged = string.Empty;
|
||||
return false;
|
||||
}
|
||||
|
||||
[GeneratedRegex(@"^(?:#{1,6}\s|[-*+]\s|\d+\.\s|>\s|```)", RegexOptions.Singleline)]
|
||||
private static partial Regex MarkdownBlockPrefixRegex();
|
||||
private static bool TryMergeByOverlap(string current, string incoming, out string merged)
|
||||
{
|
||||
int overlapLength = ComputeSuffixPrefixOverlap(current, incoming);
|
||||
if (overlapLength == 0)
|
||||
{
|
||||
merged = string.Empty;
|
||||
return false;
|
||||
}
|
||||
|
||||
merged = current + incoming[overlapLength..];
|
||||
return true;
|
||||
}
|
||||
|
||||
[System.Text.RegularExpressions.GeneratedRegex("[a-z0-9]+", System.Text.RegularExpressions.RegexOptions.IgnoreCase)]
|
||||
private static partial System.Text.RegularExpressions.Regex TokenRegex();
|
||||
}
|
||||
|
||||
@@ -37,22 +37,36 @@ public sealed class StreamingTextMergerTests
|
||||
Assert.Equal(incoming, StreamingTextMerger.Merge(current, incoming));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Merge_InsertsWhitespaceWhenSnapshotLikeUpdatesWouldOtherwiseGlueWordsTogether()
|
||||
[Theory]
|
||||
[InlineData("requires all wr", "itable fields", "requires all writable fields")]
|
||||
[InlineData("becomes frag", "ile for clients", "becomes fragile for clients")]
|
||||
[InlineData("Endpoint (domain) uniqu", "eness across tenants", "Endpoint (domain) uniqueness across tenants")]
|
||||
[InlineData("The doc says \"wildc", "ards are allowed\"", "The doc says \"wildcards are allowed\"")]
|
||||
[InlineData("What wildcard syntax supported (*.cont", "oso.com? contoso.* ?)", "What wildcard syntax supported (*.contoso.com? contoso.* ?)")]
|
||||
[InlineData("How does Pur", "view match traffic", "How does Purview match traffic")]
|
||||
[InlineData("more M", "DA properties", "more MDA properties")]
|
||||
[InlineData("does UA", "G normalize them?", "does UAG normalize them?")]
|
||||
public void Merge_DoesNotInjectSpacesIntoSplitWords(string current, string incoming, string expected)
|
||||
{
|
||||
Assert.Equal(
|
||||
"How about The **Ashen Crown** feels",
|
||||
StreamingTextMerger.Merge("How about", "The **Ashen Crown** feels"));
|
||||
Assert.Equal(
|
||||
"The **Ashen Crown** feels classic and timeless.",
|
||||
StreamingTextMerger.Merge("The **Ashen Crown** feels", "classic and timeless."));
|
||||
Assert.Equal(expected, StreamingTextMerger.Merge(current, incoming));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Merge_InsertsNewlineBeforeStreamedMarkdownBlockMarkers()
|
||||
public void Merge_PreservesWhitespaceAlreadyPresentInDelta()
|
||||
{
|
||||
Assert.Equal(
|
||||
"How about The **Ashen Crown** feels",
|
||||
StreamingTextMerger.Merge("How about", " The **Ashen Crown** feels"));
|
||||
Assert.Equal(
|
||||
"The **Ashen Crown** feels classic and timeless.",
|
||||
StreamingTextMerger.Merge("The **Ashen Crown** feels", " classic and timeless."));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Merge_PreservesNewlineAlreadyPresentInDelta()
|
||||
{
|
||||
Assert.Equal(
|
||||
"If you want, I can also give you\n- darker titles",
|
||||
StreamingTextMerger.Merge("If you want, I can also give you", "- darker titles"));
|
||||
StreamingTextMerger.Merge("If you want, I can also give you", "\n- darker titles"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5,52 +5,6 @@ function tokenize(value: string): string[] {
|
||||
.filter((token) => token.length > 0);
|
||||
}
|
||||
|
||||
function shouldInsertNewlineBoundary(current: string, incoming: string): boolean {
|
||||
if (current.endsWith('\n')) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return /^(?:#{1,6}\s|[-*+]\s|\d+\.\s|>\s|```)/.test(incoming.trimStart());
|
||||
}
|
||||
|
||||
function shouldInsertSpaceBoundary(current: string, incoming: string): boolean {
|
||||
const lastChar = current.at(-1);
|
||||
const firstChar = incoming[0];
|
||||
if (!lastChar || !firstChar) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (/\s/.test(lastChar) || /\s/.test(firstChar) || /[([{/"'`]/.test(lastChar)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (/^[.,!?;:%)\]}]/.test(firstChar)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (/^[*_`~\[]/.test(firstChar) || /^[A-Z0-9]/.test(firstChar)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
const currentTokens = tokenize(current);
|
||||
const incomingTokens = tokenize(incoming);
|
||||
const firstIncomingToken = incomingTokens[0] ?? '';
|
||||
|
||||
return currentTokens.length >= 2 && incomingTokens.length >= 2 && firstIncomingToken.length >= 2;
|
||||
}
|
||||
|
||||
function appendWithNaturalBoundary(current: string, incoming: string): string {
|
||||
if (shouldInsertNewlineBoundary(current, incoming)) {
|
||||
return `${current}\n${incoming}`;
|
||||
}
|
||||
|
||||
if (shouldInsertSpaceBoundary(current, incoming)) {
|
||||
return `${current} ${incoming}`;
|
||||
}
|
||||
|
||||
return current + incoming;
|
||||
}
|
||||
|
||||
function computeSuffixPrefixOverlap(current: string, incoming: string): number {
|
||||
const maxOverlap = Math.min(current.length, incoming.length);
|
||||
for (let length = maxOverlap; length > 0; length -= 1) {
|
||||
@@ -109,5 +63,5 @@ export function mergeStreamingText(current: string, incoming: string): string {
|
||||
return incoming;
|
||||
}
|
||||
|
||||
return appendWithNaturalBoundary(current, incoming);
|
||||
return current + incoming;
|
||||
}
|
||||
|
||||
@@ -26,17 +26,34 @@ describe('streaming text merge', () => {
|
||||
expect(mergeStreamingText(current, incoming)).toBe(incoming);
|
||||
});
|
||||
|
||||
test('inserts whitespace when snapshot-like updates would otherwise glue words together', () => {
|
||||
expect(mergeStreamingText('How about', 'The **Ashen Crown** feels')).toBe(
|
||||
test.each([
|
||||
['requires all wr', 'itable fields', 'requires all writable fields'],
|
||||
['becomes frag', 'ile for clients', 'becomes fragile for clients'],
|
||||
['Endpoint (domain) uniqu', 'eness across tenants', 'Endpoint (domain) uniqueness across tenants'],
|
||||
['The doc says "wildc', 'ards are allowed"', 'The doc says "wildcards are allowed"'],
|
||||
[
|
||||
'What wildcard syntax supported (*.cont',
|
||||
'oso.com? contoso.* ?)',
|
||||
'What wildcard syntax supported (*.contoso.com? contoso.* ?)',
|
||||
],
|
||||
['How does Pur', 'view match traffic', 'How does Purview match traffic'],
|
||||
['more M', 'DA properties', 'more MDA properties'],
|
||||
['does UA', 'G normalize them?', 'does UAG normalize them?'],
|
||||
])('does not inject spaces into split words: %s + %s', (current, incoming, expected) => {
|
||||
expect(mergeStreamingText(current, incoming)).toBe(expected);
|
||||
});
|
||||
|
||||
test('preserves whitespace already present in delta', () => {
|
||||
expect(mergeStreamingText('How about', ' The **Ashen Crown** feels')).toBe(
|
||||
'How about The **Ashen Crown** feels',
|
||||
);
|
||||
expect(mergeStreamingText('The **Ashen Crown** feels', 'classic and timeless.')).toBe(
|
||||
expect(mergeStreamingText('The **Ashen Crown** feels', ' classic and timeless.')).toBe(
|
||||
'The **Ashen Crown** feels classic and timeless.',
|
||||
);
|
||||
});
|
||||
|
||||
test('inserts a newline before streamed markdown block markers', () => {
|
||||
expect(mergeStreamingText('If you want, I can also give you', '- darker titles')).toBe(
|
||||
test('preserves newline already present in delta', () => {
|
||||
expect(mergeStreamingText('If you want, I can also give you', '\n- darker titles')).toBe(
|
||||
'If you want, I can also give you\n- darker titles',
|
||||
);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user