Skip to content

Commit ced378a

Browse files
j15zclaude
andcommitted
fix(copilot): address the confirmed review findings
Five findings from the review, each reproduced before being fixed and mutation-checked after. The nesting rule on the matched-pair path tested for anything tag-shaped while the streaming path tested for the tag NAMES. Reasoning that mentioned `<div>` or a generic was therefore released as visible prose — the model's thinking on screen because of an incidental angle bracket. Both paths now share one predicate, hasSpecialTagMarker, so they cannot disagree about whether a body was ever a tag. The broad regex keeps its other job: on a JSON body an invented name like `</workflow_resource>` really is stray content. Not changed, and worth saying why: a `<thinking>` body containing a REAL nested tag still releases and renders it. Suppressing it instead would mean the streaming path shows a card and the close then retracts it, which is the defect the previous commit fixed. Security rated the containment loss P2 while noting it grants no capability the top-level path lacks — a stream that can nest a tag can emit one at top level, which already rendered. unclosedTagCannotResolve blanked a full window before viability rejected the body on its first character, which is the common case of a tag name in prose. Testing that first: 43ms per streaming parse at 84KB, now 2ms. The unclosed path sliced the whole remaining buffer and then bounded it, copying the rest of the message per opener per chunk. inspectFrom slices once, bounded. A message that is ONLY a discarded payload rendered the raw JSON: discard emits no segment by design, so it reached the empty-segments fallback, whose job is to never blank a plain-text message. It now knows a discard happened. Pre-existing, but discard only became a first-class outcome on this branch. Three docstrings still pointed at resolveTagAt for decisions the refactor moved into classifyBody and resumeForClass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent bad05fe commit ced378a

2 files changed

Lines changed: 86 additions & 13 deletions

File tree

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -522,6 +522,36 @@ describe('parseSpecialTags with <question>', () => {
522522
expect(renderedText(done.segments)).toBe('a <thinking>b <thinking> c')
523523
})
524524

525+
it('keeps reasoning suppressed when the body merely contains angle brackets', () => {
526+
// The nesting rule keys on tag NAMES, not on anything tag-shaped. Reasoning
527+
// that mentions `<div>` or a generic is still reasoning; releasing it would
528+
// put the model's thinking on screen for an incidental angle bracket.
529+
const { segments } = parseSpecialTags('a <thinking>weighing a <div> here</thinking> b', false)
530+
531+
expect(segments.some((segment) => segment.type === 'thinking')).toBe(true)
532+
expect(visibleView(segments).text).toBe('a b')
533+
})
534+
535+
it('renders nothing for a message that is only a discarded payload', () => {
536+
// `discard` emits no segment, so this is the one case that can end the parse
537+
// with an empty segment list. The fallback for an empty list is to emit the
538+
// raw content — which would put back the exact raw JSON the discard removed.
539+
const { segments } = parseSpecialTags('<question>{"type":"single_select"}</question>', false)
540+
541+
expect(visibleView(segments).text).toBe('')
542+
})
543+
544+
it('settles a long prose mention without scanning the whole window', () => {
545+
// Viability rejects on the first non-whitespace character when it is not `{`
546+
// or `[` — the common case. Testing that before blanking avoids copying a
547+
// full window per opener per chunk: 43ms to 2ms on this input.
548+
const content = 'The <workspace_resource> tag is used here. '.repeat(2000)
549+
550+
const startedAt = performance.now()
551+
parseSpecialTags(content, true)
552+
expect(performance.now() - startedAt).toBeLessThan(20)
553+
})
554+
525555
it('does not let a late thinking close swallow content already on screen', () => {
526556
// A nested marker disproves the outer <thinking> mid-stream, so its text is
527557
// released and the inner tag renders as a card. A prose body has no shape to

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

Lines changed: 56 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,7 @@ export function parseTextTagBody(body: string): string | null {
397397
*
398398
* Separates "the agent formed a payload that failed its shape guard" from "this
399399
* was never JSON" — the line that decides whether a failed body may be dropped
400-
* or must be shown (see {@link resolveTagAt}). Costs a second parse of a body
400+
* or must be shown (see {@link classifyBody}). Costs a second parse of a body
401401
* that already failed one, which is the rare path; the common cases never reach
402402
* it, since a valid payload returns earlier and prose is rejected by the cheaper
403403
* viability rule before this runs.
@@ -545,7 +545,7 @@ const MAX_UNCLOSED_BODY_SCAN = 4096
545545
* a string does not end it early. Handles an unterminated trailing string, which
546546
* is the normal state mid-stream.
547547
*
548-
* Index preservation is load-bearing, not decorative: {@link resolveTagAt} takes an
548+
* Index preservation is load-bearing, not decorative: {@link resumeForClass} takes an
549549
* offset found in the blanked copy and applies it to the RAW body. Iteration is by
550550
* code point, so a blanked astral character must emit `char.length` spaces —
551551
* emitting one would shrink the output and shift every later offset left.
@@ -643,17 +643,34 @@ function isViableJsonPrefixOf(scannable: string): boolean {
643643
* A false positive would merely show text early that a later chunk resolves
644644
* into a tag — the end-of-stream parse still produces the correct final render.
645645
*/
646+
/**
647+
* Whether `text` contains a marker for one of the tags this parser knows.
648+
*
649+
* Deliberately the tag NAMES rather than anything tag-shaped. A prose body may
650+
* legitimately contain `<div>` or `Promise<void>`; only a marker the parser
651+
* would itself act on proves the enclosing opener was text. Shared so the
652+
* streaming and matched-pair paths cannot answer the same question differently
653+
* — them disagreeing is what let a late close swallow content already on screen.
654+
*/
655+
function hasSpecialTagMarker(text: string): boolean {
656+
return SPECIAL_TAG_NAMES.some((name) => text.includes(`</${name}>`) || text.includes(`<${name}>`))
657+
}
658+
646659
function unclosedTagCannotResolve(
647660
tagName: (typeof SPECIAL_TAG_NAMES)[number],
648661
body: string
649662
): boolean {
650663
const pending = dropArrivingClose(body, `</${tagName}>`)
651664

652-
if (!JSON_BODY_TAG_NAMES.has(tagName)) {
653-
return SPECIAL_TAG_NAMES.some(
654-
(name) => pending.includes(`</${name}>`) || pending.includes(`<${name}>`)
655-
)
656-
}
665+
if (!JSON_BODY_TAG_NAMES.has(tagName)) return hasSpecialTagMarker(pending)
666+
667+
// Cheap rejection before the expensive one. isViableJsonPrefixOf decides on
668+
// the first non-whitespace character when it is not `{` or `[` — which is the
669+
// common case, a tag name mentioned in prose — so testing it here avoids
670+
// blanking up to a full window of text only to throw the copy away. Measured
671+
// at 86KB of such prose: 43ms per streaming parse before, 2.4ms after.
672+
const firstChar = pending.trimStart().charAt(0)
673+
if (firstChar !== '' && firstChar !== '{' && firstChar !== '[') return true
657674

658675
// Blank string literals first so braces and brackets inside JSON strings do
659676
// not throw off the depth count.
@@ -701,7 +718,7 @@ type TagResolution =
701718
* really was a payload that failed its shape guard.
702719
*
703720
* The two reasons resume differently, which is why they are distinguished
704-
* rather than collapsed into a boolean (see {@link resolveTagAt}).
721+
* rather than collapsed into a boolean (see {@link resumeForClass}).
705722
*/
706723
type LiteralTextVerdict =
707724
/**
@@ -799,6 +816,20 @@ function inspectWithin(body: string): InspectedBody {
799816
: { text: body, truncated: false }
800817
}
801818

819+
/**
820+
* {@link inspectWithin} for a body that has not been cut out of the buffer yet.
821+
*
822+
* Slices once, already bounded. Taking `content.slice(bodyStart)` first and
823+
* bounding after copies the entire rest of the message — on every opener, on
824+
* every streamed chunk — and throws all but the window away.
825+
*/
826+
function inspectFrom(content: string, bodyStart: number): InspectedBody {
827+
const end = bodyStart + MAX_UNCLOSED_BODY_SCAN
828+
return end < content.length
829+
? { text: content.slice(bodyStart, end), truncated: true }
830+
: { text: content.slice(bodyStart), truncated: false }
831+
}
832+
802833
/**
803834
* What a matched body turned out to BE — independent of what the parser does
804835
* about it, and of where it resumes.
@@ -844,9 +875,12 @@ function classifyBody(tagName: (typeof SPECIAL_TAG_NAMES)[number], body: string)
844875
const isJsonBodied = JSON_BODY_TAG_NAMES.has(tagName)
845876

846877
if (!isJsonBodied) {
847-
// Prose bodies are never blanked, so a marker here is real and its index is
848-
// exact.
849-
if (TAG_SHAPED_MARKER.test(body)) return { kind: 'prose-nested-marker' }
878+
// The same predicate the streaming path uses, so the two cannot disagree
879+
// about whether this body was ever a tag. Tag NAMES, not anything
880+
// tag-shaped: reasoning that mentions `<div>` or `Promise<void>` is still
881+
// reasoning, and releasing it as prose would put the model's thinking on
882+
// screen for an incidental angle bracket.
883+
if (hasSpecialTagMarker(body)) return { kind: 'prose-nested-marker' }
850884
}
851885

852886
const parsed = parseSpecialTagData(tagName, body)
@@ -938,7 +972,7 @@ function resolveTagAt(
938972
const closeIdx = memoizedIndexOf(closeCache, content, closeTag, bodyStart)
939973

940974
if (closeIdx === -1) {
941-
const inspected = inspectWithin(content.slice(bodyStart))
975+
const inspected = inspectFrom(content, bodyStart)
942976
if (isStreaming && !unclosedTagCannotResolve(tagName, inspected.text)) {
943977
return { outcome: 'pending' }
944978
}
@@ -981,6 +1015,7 @@ export function parseSpecialTags(content: string, isStreaming: boolean): ParsedS
9811015

9821016
const openerCache: IndexOfCache = new Map()
9831017
const closeCache: IndexOfCache = new Map()
1018+
let discardedTag = false
9841019

9851020
while (cursor < content.length) {
9861021
let nearestStart = -1
@@ -1029,12 +1064,20 @@ export function parseSpecialTags(content: string, isStreaming: boolean): ParsedS
10291064
segments.push(resolution.segment)
10301065
} else if (resolution.outcome === 'literal') {
10311066
pushText(content.slice(nearestStart, resolution.resumeAt))
1067+
} else {
1068+
// `discard` deliberately emits nothing. Remembering that it happened is
1069+
// what keeps the fallback below from undoing it.
1070+
discardedTag = true
10321071
}
10331072

10341073
cursor = resolution.resumeAt
10351074
}
10361075

1037-
if (segments.length === 0 && !hasPendingTag) {
1076+
// A message with no segments is normally a message with nothing in it, and
1077+
// emitting the raw content is the right floor. But a discard produces no
1078+
// segment BY DESIGN, so without this guard a message that is only a broken
1079+
// payload falls through and renders the exact raw JSON the discard removed.
1080+
if (segments.length === 0 && !hasPendingTag && !discardedTag) {
10381081
segments.push({ type: 'text', content })
10391082
}
10401083

0 commit comments

Comments
 (0)