Skip to content

Commit c61dd9d

Browse files
committed
fix(copilot): stop the literal path flashing payloads and rescanning the buffer
Review round on the four-outcome rewrite. Three defects in how an opener is judged, plus the cost of judging it. A valid tag showed its raw payload as text while its closing marker streamed in. The JSON value closes at the `}`, so a half-arrived `</opt` read as stray trailing content and settled the tag as unresolvable. Every JSON-bodied tag hit this, and most replies carry a trailing <options> block. dropArrivingClose now ignores a trailing fragment that could still grow into this tag's own close. Evidence that a close is genuinely wrong still lands immediately: a misspelled `</workflow_resource>` is not a prefix of `</workspace_resource>`, and a truncated `</workspac` stops being one the moment prose follows it. bodyIsLiteralText scanned the raw body for tag markers while its streaming counterpart blanked JSON string literals first. A payload that failed its shape guard and legitimately quoted tag syntax was therefore called literal text and rendered as raw JSON, which is what discard exists to prevent. Both paths now judge the same blanked body. An opener whose own close was misspelled reached forward and matched the NEXT tag's close, swallowing a valid resource into one literal span and destroying its chip. When markers in the body prove the matched close belongs to a different opener, resolution resumes past the opener so the interior is rescanned and the inner tag still renders. Cost: resuming past a rejected opener instead of abandoning the message made each parse O(openers x length), and the parse re-runs for every streamed chunk. A 40KB reply repeatedly mentioning a tag name cost 7.3s of blocked main thread. The unclosed-body inspection is now bounded, since both rules decide on their first piece of evidence, and the opener and close lookups are memoized per parse rather than rescanning to the end of the buffer for every opener. Same message: 1.2s. A realistic 40KB reply with 8 mentions: 0.35ms per parse, 28ms across the entire stream. Also derives JSON_BODY_TAG_NAMES from SPECIAL_TAG_NAMES so a new tag cannot silently fall back to the weaker prose heuristics; deletes a docstring the rewrite orphaned above TAG_SHAPED_MARKER; corrects one still describing the first-character check that depth tracking replaced; drops a test duplicating one already in the file; and fixes a test that named the no-nesting rule but passed with that rule deleted, because its JSON closed before the marker.
1 parent ef6cc84 commit c61dd9d

2 files changed

Lines changed: 201 additions & 56 deletions

File tree

apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.test.ts

Lines changed: 60 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -218,16 +218,71 @@ describe('parseSpecialTags with <question>', () => {
218218
it('still drops a marker-free malformed payload rather than showing raw JSON', () => {
219219
// The complement of the case above: no tag markers in the body, so this is
220220
// a genuinely broken emission from the agent, not swallowed prose.
221-
const { segments } = parseSpecialTags(
221+
const { segments, hasPendingTag } = parseSpecialTags(
222222
'Before. <question>{"type":"single_select"}</question> After.',
223223
false
224224
)
225+
expect(hasPendingTag).toBe(false)
225226
expect(segments).toEqual([
226227
{ type: 'text', content: 'Before. ' },
227228
{ type: 'text', content: ' After.' },
228229
])
229230
})
230231

232+
it('drops that same payload even when its JSON quotes tag syntax', () => {
233+
// The marker scan must blank JSON strings the way the streaming path does.
234+
// Scanning the raw body sees `</options>` inside the prompt, calls the span
235+
// literal text, and renders the raw payload — the outcome `discard` exists
236+
// to prevent.
237+
const { segments } = parseSpecialTags(
238+
'A <question>[{"type":"single_select","prompt":"use </options> here?"}]</question> B',
239+
false
240+
)
241+
expect(segments).toEqual([
242+
{ type: 'text', content: 'A ' },
243+
{ type: 'text', content: ' B' },
244+
])
245+
})
246+
247+
it('does not flash the payload while the closing tag is still arriving', () => {
248+
// Each frame below is a real mid-stream state: the JSON value has closed, so
249+
// without tolerating an arriving close the trailing `</opt` reads as stray
250+
// content and the whole payload is released as text until the final `>`.
251+
for (const fragment of ['<', '</', '</o', '</opt', '</options']) {
252+
const { segments, hasPendingTag } = parseSpecialTags(
253+
`see <options>[{"title":"a","description":"b"}]${fragment}`,
254+
true
255+
)
256+
expect(hasPendingTag).toBe(true)
257+
expect(segments.map((s) => ('content' in s ? s.content : '')).join('')).toBe('see ')
258+
}
259+
})
260+
261+
it('still rejects a close whose name is wrong rather than merely unfinished', () => {
262+
// The counterpart to the case above: `</workflow_resource>` can never grow
263+
// into `</workspace_resource>`, so it settles immediately instead of hiding
264+
// the rest of the message for the remainder of the stream.
265+
const { hasPendingTag } = parseSpecialTags(
266+
'see <workspace_resource>{"type":"file","path":"a.md"}</workflow_resource> and then prose.',
267+
true
268+
)
269+
expect(hasPendingTag).toBe(false)
270+
})
271+
272+
it('keeps a valid tag whose close an earlier broken tag would borrow', () => {
273+
// The first opener misspells its close, so it reaches forward and matches
274+
// the SECOND tag's close, swallowing a perfectly good resource into one
275+
// literal span. Resuming past the opener re-scans the interior instead.
276+
const raw =
277+
'See <workspace_resource>{"type":"file","path":"a.md"}</workflow_resource>\n' +
278+
'and a real one: <workspace_resource>{"type":"file","path":"b.md","title":"b"}</workspace_resource>'
279+
const { segments } = parseSpecialTags(raw, false)
280+
expect(segments.some((s) => s.type === 'workspace_resource')).toBe(true)
281+
expect(segments.map((s) => ('content' in s ? s.content : '')).join('')).toContain(
282+
'</workflow_resource>'
283+
)
284+
})
285+
231286
it('still renders a matched pair whose body IS valid', () => {
232287
const raw =
233288
'see <workspace_resource>{"type":"file","path":"files/a.md","title":"a.md"}</workspace_resource> ok'
@@ -275,10 +330,10 @@ describe('parseSpecialTags with <question>', () => {
275330

276331
it('bails when a foreign closing tag appears inside the body', () => {
277332
// Tags never nest, so a close for a different tag proves the opener was text.
278-
const { hasPendingTag } = parseSpecialTags(
279-
'see <options>[{"title":"a","description":"b"}] </question> more',
280-
true
281-
)
333+
// The array is left UNCLOSED on purpose: with a closed value the JSON rule
334+
// would independently reject the trailing content, and this test would still
335+
// pass with the nesting rule deleted. Unclosed, only nesting can explain it.
336+
const { hasPendingTag } = parseSpecialTags('see <options>[{"title":"a" </question> more', true)
282337
expect(hasPendingTag).toBe(false)
283338
})
284339

@@ -349,18 +404,6 @@ describe('parseSpecialTags with <question>', () => {
349404
expect(hasPendingTag).toBe(true)
350405
expect(segments).toEqual([{ type: 'text', content: 'Let me ask. ' }])
351406
})
352-
353-
it('drops a question tag with an invalid body but keeps surrounding text', () => {
354-
const { segments, hasPendingTag } = parseSpecialTags(
355-
'Before. <question>{"type":"single_select"}</question> After.',
356-
false
357-
)
358-
expect(hasPendingTag).toBe(false)
359-
expect(segments).toEqual([
360-
{ type: 'text', content: 'Before. ' },
361-
{ type: 'text', content: ' After.' },
362-
])
363-
})
364407
})
365408

366409
describe('service_account credential tag', () => {

apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.tsx

Lines changed: 141 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -464,34 +464,43 @@ function parseSpecialTagData(
464464
return null
465465
}
466466

467-
/**
468-
* Parses inline special tags (`<options>`, `<usage_upgrade>`, `<workspace_resource>`) from streamed
469-
* text content. Complete tags are extracted into typed segments; incomplete
470-
* tags (still streaming) are suppressed from display and flagged via
471-
* `hasPendingTag` so the caller can show a loading indicator.
472-
*
473-
* Trailing partial opening tags (e.g. `<opt`, `<usage_`) are also stripped
474-
* during streaming to prevent flashing raw markup.
475-
*/
476467
/**
477468
* Any tag-shaped marker, including names that are not special tags at all — the
478469
* model inventing `</workflow_resource>` is exactly the case that matters.
479470
*/
480471
const TAG_SHAPED_MARKER = /<\/?[a-zA-Z][\w-]*>/
481472

482473
/**
483-
* Tags whose body must be JSON. `thinking` is the exception — its body is prose
484-
* (see {@link parseTextTagBody}), so a non-JSON body there says nothing about
485-
* whether a close is still coming.
474+
* The one tag whose body is prose rather than JSON (see {@link parseTextTagBody}),
475+
* so a non-JSON body there says nothing about whether a close is still coming.
486476
*/
487-
const JSON_BODY_TAG_NAMES = new Set<(typeof SPECIAL_TAG_NAMES)[number]>([
488-
'options',
489-
'usage_upgrade',
490-
'credential',
491-
'mothership-error',
492-
'workspace_resource',
493-
'question',
494-
])
477+
const PROSE_BODY_TAG_NAME: (typeof SPECIAL_TAG_NAMES)[number] = 'thinking'
478+
479+
/**
480+
* Tags whose body must be JSON.
481+
*
482+
* Derived from {@link SPECIAL_TAG_NAMES} rather than hand-listed: a new tag is
483+
* JSON-bodied by default, so forgetting to update this set cannot silently
484+
* downgrade it to the weaker prose heuristics. Opting a tag out is an explicit
485+
* edit to {@link PROSE_BODY_TAG_NAME}.
486+
*/
487+
const JSON_BODY_TAG_NAMES: ReadonlySet<(typeof SPECIAL_TAG_NAMES)[number]> = new Set(
488+
SPECIAL_TAG_NAMES.filter((name) => name !== PROSE_BODY_TAG_NAME)
489+
)
490+
491+
/**
492+
* How much of an unclosed body to inspect per parse.
493+
*
494+
* Both rules in {@link unclosedTagCannotResolve} decide on their FIRST piece of
495+
* evidence — the first foreign marker, or the first character that breaks JSON
496+
* viability — so a bounded window reaches the same verdict as the full remainder
497+
* for any real payload. Unbounded, the check is O(remaining length) and runs
498+
* once per opener inside a parse that re-runs for every streamed chunk, so a
499+
* long reply repeatedly mentioning a tag name in prose costs seconds of
500+
* main-thread time. Evidence past the window only defers the decision to a later
501+
* chunk, which is the same conservative direction the rules already take.
502+
*/
503+
const MAX_UNCLOSED_BODY_SCAN = 4096
495504

496505
/**
497506
* Strip the contents of JSON string literals from `body`, replacing them with
@@ -544,7 +553,15 @@ function blankJsonStringLiterals(body: string): string {
544553
* do not affect the depth count.
545554
*/
546555
function isViableJsonPrefix(body: string): boolean {
547-
const scannable = blankJsonStringLiterals(body)
556+
return isViableJsonPrefixOf(blankJsonStringLiterals(body))
557+
}
558+
559+
/**
560+
* {@link isViableJsonPrefix} for a body whose string literals are already
561+
* blanked. Callers that had to blank the body for their own scan pass the result
562+
* straight through instead of paying for a second pass over the same text.
563+
*/
564+
function isViableJsonPrefixOf(scannable: string): boolean {
548565
if (scannable.trim() === '') return true
549566

550567
const firstChar = scannable.trimStart().charAt(0)
@@ -577,8 +594,10 @@ function isViableJsonPrefix(body: string): boolean {
577594
*
578595
* 1. Tags never nest. Any other special-tag marker inside the body — opening or
579596
* closing — means this opener was literal text, not a real tag.
580-
* 2. JSON-bodied tags must start with `{` or `[`. The first non-space character
581-
* settles it, so prose after the marker is caught on the very next chunk.
597+
* 2. A JSON body must stay a viable JSON prefix. Depth is tracked rather than
598+
* testing the first character alone, so a body whose top-level value has
599+
* already closed is caught the moment stray content follows it — including a
600+
* misspelled close like `</workflow_resource>`, which no marker rule sees.
582601
*
583602
* Both are conservative: they only fire on content that could not have parsed.
584603
* A false positive would merely show text early that a later chunk resolves
@@ -588,22 +607,44 @@ function unclosedTagCannotResolve(
588607
tagName: (typeof SPECIAL_TAG_NAMES)[number],
589608
body: string
590609
): boolean {
610+
const pending = dropArrivingClose(body, `</${tagName}>`)
591611
// For a JSON-bodied tag, ignore markers inside string literals: quoting tag
592612
// syntax is legitimate content, and treating it as evidence would bail on a
593613
// tag that goes on to close correctly — showing raw JSON that then snaps into
594614
// a rendered card.
595-
const scannable = JSON_BODY_TAG_NAMES.has(tagName) ? blankJsonStringLiterals(body) : body
615+
const isJsonBodied = JSON_BODY_TAG_NAMES.has(tagName)
616+
const scannable = isJsonBodied ? blankJsonStringLiterals(pending) : pending
596617
for (const name of SPECIAL_TAG_NAMES) {
597618
// A close for this tag is absent by definition here, so this catches a
598619
// FOREIGN close; the open check catches nesting, including self-nesting.
599620
if (scannable.includes(`</${name}>`) || scannable.includes(`<${name}>`)) return true
600621
}
601622

602-
if (JSON_BODY_TAG_NAMES.has(tagName) && !isViableJsonPrefix(body)) return true
623+
if (isJsonBodied && !isViableJsonPrefixOf(scannable)) return true
603624

604625
return false
605626
}
606627

628+
/**
629+
* Drop a trailing fragment that could still grow into `closeTag`.
630+
*
631+
* Mid-stream the closing marker arrives a character at a time, so a body sits at
632+
* `]</opt` for several frames before `</options>` completes. That fragment is an
633+
* arriving close, not stray content — counting it as fatal is what made a
634+
* perfectly valid tag show its raw payload as text until the final `>` landed.
635+
*
636+
* Only a fragment at the very END is dropped, so evidence that the close is
637+
* genuinely wrong still lands immediately: a misspelled `</workflow_resource>`
638+
* is not a prefix of `</workspace_resource>`, and a truncated `</workspac`
639+
* followed by prose stops being one the moment the prose arrives.
640+
*/
641+
function dropArrivingClose(body: string, closeTag: string): string {
642+
for (let n = Math.min(closeTag.length - 1, body.length); n > 0; n--) {
643+
if (body.endsWith(closeTag.slice(0, n))) return body.slice(0, -n)
644+
}
645+
return body
646+
}
647+
607648
/**
608649
* How one opening tag resolved. Naming the four outcomes is the point: the
609650
* parser previously decided each case inline, which is how "drop it" quietly
@@ -620,30 +661,76 @@ type TagResolution =
620661
| { outcome: 'pending' }
621662

622663
/**
623-
* True when a failed body was never an attempted payload — so the markers were
624-
* literal text and the span must be shown rather than swallowed.
664+
* Why a failed body was never an attempted payload — so the markers were literal
665+
* text and the span must be shown rather than swallowed. `null` means the body
666+
* really was a payload that failed its shape guard.
667+
*
668+
* The two reasons resume differently, which is why they are distinguished
669+
* rather than collapsed into a boolean (see {@link resolveTagAt}).
670+
*/
671+
type LiteralTextReason =
672+
/** The body carries tag markers, so the close we matched belongs elsewhere. */
673+
| 'foreign-markers'
674+
/** The tag wrapped prose that was never JSON to begin with. */
675+
| 'never-a-payload'
676+
677+
function literalTextReason(
678+
tagName: (typeof SPECIAL_TAG_NAMES)[number],
679+
body: string
680+
): LiteralTextReason | null {
681+
const isJsonBodied = JSON_BODY_TAG_NAMES.has(tagName)
682+
// Markers inside a JSON string are content, not evidence — a `<question>` may
683+
// legitimately quote tag syntax in its prompt. Scanning the raw body here
684+
// would classify a broken payload as literal text and render it as raw JSON,
685+
// which is exactly what `discard` exists to prevent. Mirrors the same blanking
686+
// in unclosedTagCannotResolve, which judges the same body mid-stream.
687+
const scannable = isJsonBodied ? blankJsonStringLiterals(body) : body
688+
if (TAG_SHAPED_MARKER.test(scannable)) return 'foreign-markers'
689+
if (isJsonBodied && !isViableJsonPrefixOf(scannable)) return 'never-a-payload'
690+
return null
691+
}
692+
693+
/**
694+
* `content.indexOf(needle, from)` memoized per needle.
625695
*
626-
* Either the close we matched belongs to a different opener (the body carries
627-
* tag markers), or the tag wrapped prose that was never JSON to begin with.
696+
* The opener scan and the close lookup search the same handful of markers over
697+
* and over as the cursor advances. A needle absent from the message resolves to
698+
* -1 once and is never searched again; a present one is re-searched only when
699+
* the cursor passes its last hit. Unmemoized, each lookup rescans to the end of
700+
* the buffer for every opener, which is quadratic on a message that mentions a
701+
* tag name many times — and this parse re-runs for every streamed chunk.
702+
*
703+
* Safe because `from` only ever increases within a parse: a cached -1 stays -1,
704+
* and a cached hit at or after `from` is still the first hit at or after `from`.
628705
*/
629-
function bodyIsLiteralText(tagName: (typeof SPECIAL_TAG_NAMES)[number], body: string): boolean {
630-
if (TAG_SHAPED_MARKER.test(body)) return true
631-
return JSON_BODY_TAG_NAMES.has(tagName) && !isViableJsonPrefix(body)
706+
function memoizedIndexOf(
707+
cache: Map<string, number>,
708+
content: string,
709+
needle: string,
710+
from: number
711+
): number {
712+
const cached = cache.get(needle)
713+
if (cached !== undefined && (cached === -1 || cached >= from)) return cached
714+
const idx = content.indexOf(needle, from)
715+
cache.set(needle, idx)
716+
return idx
632717
}
633718

634719
function resolveTagAt(
635720
content: string,
636721
openIndex: number,
637722
tagName: (typeof SPECIAL_TAG_NAMES)[number],
638-
isStreaming: boolean
723+
isStreaming: boolean,
724+
closeCache: Map<string, number>
639725
): TagResolution {
640726
const openTag = `<${tagName}>`
641727
const closeTag = `</${tagName}>`
642728
const bodyStart = openIndex + openTag.length
643-
const closeIdx = content.indexOf(closeTag, bodyStart)
729+
const closeIdx = memoizedIndexOf(closeCache, content, closeTag, bodyStart)
644730

645731
if (closeIdx === -1) {
646-
if (isStreaming && !unclosedTagCannotResolve(tagName, content.slice(bodyStart))) {
732+
const inspectable = content.slice(bodyStart, bodyStart + MAX_UNCLOSED_BODY_SCAN)
733+
if (isStreaming && !unclosedTagCannotResolve(tagName, inspectable)) {
647734
return { outcome: 'pending' }
648735
}
649736
// Nothing can close it, so only the opener itself is literal. Resuming just
@@ -658,7 +745,16 @@ function resolveTagAt(
658745
const parsed = parseSpecialTagData(tagName, body)
659746
if (parsed) return { outcome: 'segment', segment: parsed, resumeAt }
660747

661-
if (bodyIsLiteralText(tagName, body)) return { outcome: 'literal', resumeAt }
748+
const reason = literalTextReason(tagName, body)
749+
if (reason === 'foreign-markers') {
750+
// A marker in the body proves the close we matched opened somewhere else —
751+
// this opener reached past its own missing close and borrowed a later tag's.
752+
// Resuming past the OPENER instead of past that borrowed close re-scans the
753+
// interior, so the genuine tag inside still renders instead of being
754+
// swallowed into one literal span.
755+
return { outcome: 'literal', resumeAt: bodyStart }
756+
}
757+
if (reason === 'never-a-payload') return { outcome: 'literal', resumeAt }
662758

663759
// A well-formed value that failed its shape guard is a broken emission from
664760
// the agent; showing the user raw JSON there would be worse than nothing.
@@ -667,7 +763,10 @@ function resolveTagAt(
667763

668764
/**
669765
* Splits streamed text into renderable segments, extracting complete special
670-
* tags and deciding what to do with the ones that never resolve.
766+
* tags and deciding what to do with the ones that never resolve. Incomplete
767+
* tags are suppressed and flagged via `hasPendingTag` so the caller can show a
768+
* loading indicator, and a trailing partial opening marker (`<opt`, `<usage_`)
769+
* is stripped during streaming so it never flashes as raw markup.
671770
*
672771
* Adjacent text segments are concatenated by the renderer, so emitting a span
673772
* as several pieces is display-neutral.
@@ -681,12 +780,15 @@ export function parseSpecialTags(content: string, isStreaming: boolean): ParsedS
681780
if (text.trim()) segments.push({ type: 'text', content: text })
682781
}
683782

783+
const openerCache = new Map<string, number>()
784+
const closeCache = new Map<string, number>()
785+
684786
while (cursor < content.length) {
685787
let nearestStart = -1
686788
let nearestTagName: (typeof SPECIAL_TAG_NAMES)[number] | '' = ''
687789

688790
for (const name of SPECIAL_TAG_NAMES) {
689-
const idx = content.indexOf(`<${name}>`, cursor)
791+
const idx = memoizedIndexOf(openerCache, content, `<${name}>`, cursor)
690792
if (idx !== -1 && (nearestStart === -1 || idx < nearestStart)) {
691793
nearestStart = idx
692794
nearestTagName = name
@@ -717,7 +819,7 @@ export function parseSpecialTags(content: string, isStreaming: boolean): ParsedS
717819

718820
pushText(content.slice(cursor, nearestStart))
719821

720-
const resolution = resolveTagAt(content, nearestStart, nearestTagName, isStreaming)
822+
const resolution = resolveTagAt(content, nearestStart, nearestTagName, isStreaming, closeCache)
721823

722824
if (resolution.outcome === 'pending') {
723825
hasPendingTag = true

0 commit comments

Comments
 (0)