fix(protocol): keep a label closer only where it is matched or continues the list - #9341
Conversation
…ues the list
The `[OPTIONS:]` label body admitted `]` unconditionally, so it ran to the
LAST closer in range instead of the first plausible one. A turn whose final
line mentions a bracket after the marker then matched across BOTH:
Use [OPTIONS: A | B] then check arr[0]
matched whole, with `labels` = ` A | B] then check arr[0`. Every consumer
removes the whole match -- `slack.format` and `messaging.renderer` cut the
visible text at `match.start()`, and `whatsapp.turn_renderer` PERSISTS the
cut turn -- so the sentence vanished from the message and came back as a
pill label. Silent data loss in user-facing text, not a rendering glitch,
and the swallowed prose was simultaneously offered as a button.
Under TRAILER (`DOTALL`, `\Z`-anchored, serving the Discord/Telegram/WeCom
renderers) the body crossed blank lines too, so the whole closing paragraph
went with it -- and with no leading prose the visible text became the empty
string.
It fires on ordinary agent output: a turn ending in a file path, an array
index, a type annotation or a footnote marker is common, and the marker rule
puts `[OPTIONS:]` on that same final line by design.
THE RULE. A closer stays in a label under EITHER of two conditions -- it is
MATCHED by a `[` earlier in the same label, or the label list CONTINUES
after it. Neither alone is enough, and each of the three shapes is pinned by
a test:
[OPTIONS: Fix [x] logging | Skip] matched pair -> parses
[OPTIONS: Alpha ] | Bravo ]] list continues -> parses
Use [OPTIONS: A | B] then check arr[0] neither -> declined
Continuation alone is the obvious rule and it is WRONG: `Fix [x] logging` is
a first-class supported shape, pinned by `test_parse_options.py` and
`test_options_buttons.py`, whose comments say so outright -- and there the
closer is followed by an ordinary word. Matching alone is equally wrong:
`[OPTIONS: Alpha ] | Bravo ]]` carries closers with no opener. Only the
union separates all three.
The continuation half is the SAME discriminator the streaming probe already
applied (`CONTINUES_LABELS_RE` in optionMarker.ts) to decide whether an
arriving closer ended the marker, so the grammar and the probe stop
answering one question two different ways.
DISJOINTNESS IS THE LINEARITY ARGUMENT. Two alternatives that both begin at
`[` would be a ReDoS shape if they could consume the same span. They cannot:
the matched form requires that its closer NOT be followed by a separator or
another closer, which is exactly when the continuation form applies. So the
four body alternatives are mutually exclusive at every position, an
unmatched `[` is left to the bare-`[` form, and each lookahead is entered
only at a bracket and bounded by the run it scans. Guards drive this
directly: six block shapes that each look like both alternatives, repeated
20,000 times and followed by a tail that fails the whole match, so the
engine must exhaust every combination it believes exists.
DECLINING MUST NOT DELETE, which is what made a latent hole load-bearing.
`messaging.renderer.split_options_trailer`'s `hide_partial` gate asks "could
this tail still be a marker in flight?" and answered it by looking for ASCII
`]` alone. Narrowing the grammar means a marker whose only closers are
lookalikes (`[OPTIONS: 【重要】修复 | 跳过】`) is now declined, falls through
to that gate, holds no ASCII `]`, and so read as in-flight and was CUT --
WeCom's sealed frame AND its persisted history entry, plus the Discord and
Telegram `self._buf = [body]` reseat, would have shown the leading prose
with the entire option list deleted and no pills to recover it from. The
gate now tests the whole `MARKER_CLOSERS` set, which is the rule
`test_hide_partial_does_not_touch_a_closed_bracket_elsewhere` already pinned
for ASCII: a tail holding a closer is not in flight, it is prose. Widening
there can only ever KEEP more text. It is a different question from
`split_trailing_protocol_suffix`'s probe, which asks whether a tail is
COMPLETE and must stay ASCII-only -- presence of a closer is not
completeness, but it is conclusive evidence of not-in-flight.
The pair form also carries the same `(?!OPTIONS:)` guard as the bare-`[`
form. Without it, it is the ONE place this rule was looser than the body it
replaced: it opens on a nested head and pairs it with that head's own
closer, so `Note [OPTIONS: see [OPTIONS: x] below | Skip]` matched from the
OUTER head -- where the old body matched nothing -- and rendered a pill
whose label is a raw protocol marker, echoed back as the user's reply when
tapped.
ACCEPTED COST, enumerated rather than summarised because there is more than
one shape and all of them parsed before. Each is a closer that satisfies
NEITHER half with ordinary words after it, where the input is genuinely
indistinguishable from "marker ended, prose followed on the same line":
[OPTIONS: Fix ]x logging | Skip] unmatched, no `[` at all
[OPTIONS: Fix list[dict[str, Any]] now | S] nesting deeper than one level
[OPTIONS: 【重要】修复 | 跳过】 a lookalike PAIR -- `【` is not
an opener, only `[` is
[OPTIONS: Fix [multi\nline] now | Skip] TRAILER only; the pair interior
excludes `\n` even under DOTALL
All four fail toward a VISIBLE marker, not toward deleted prose, and that
asymmetry is the whole reason they are affordable -- so the gate fix above
is what holds it up, and the two are pinned together in one test. Making
them parse means matching brackets to arbitrary depth and over an opener set
this grammar does not have.
OUT OF SCOPE, pinned so it is not read as a regression introduced here: the
separator-tail form, `Done. [OPTIONS: Merge | Wait], details in
CHANGELOG[1]`, still cuts to `Done.` -- byte-for-byte as it does on main.
`], ` DOES continue the label list, by the very rule that makes `[OPTIONS:
Alpha ], Bravo]` legal, so no guard applied at the internal closer can tell
the two apart. Resolving it means deciding which shape loses, which is a
separate call with its own cost.
Both backend regexes and the frontend mirror change in one commit so the
grammars cannot drift, and the body is spelled once per regex
(`_MARKER_BODY_LINE` / `_MARKER_BODY_TRAILER`) so LINE and TRAILER cannot
drift from each other either. The one body-shaped pattern deliberately NOT
derived from them is `_OPTIONS_TAIL_PREFIX_RE`, a prefix closure that has to
stay looser -- a prefix of a legal body need not be a legal body, and
`[OPTIONS: A ]` mid-stream becomes legal only once `| B]` arrives; its
docstring now says so rather than claiming to hold the grammar's body.
#9174's wrapper rules are untouched and compose with this one: a wrapped
marker carrying a continuing closer still parses. Comments that described
the old unconditional body, or that claimed a closer is readmitted in
"exactly one place" (it is two -- the pair form and the continuation form,
which is where a future `MARKER_CLOSERS` widening has to look), are
corrected rather than left to mislead.
Tests: 29 backend cases in test/test_options_marker_label_closers.py and 25
frontend cases in website/src/test/optionsMarkerLabelClosers.test.ts. Both
assert the two claims separately -- that the grammar does not MATCH, and
that the visible text is UNCHANGED -- because only the second is what the
user experiences, and `TestAcceptedCosts` carries that second claim one step
further down into `split_options_trailer`. The bug rows were measured
against a verbatim transcription of main rather than assumed: all eight
match there and delete the prose shown, while every wrapper shape #9174
added still parses here.
Fixes #9284
UX Review (Fable 5) — ✅ PASSUX-level review of The diff is confined to the marker grammar (regexes in UX-Verdict: PASS Pure grammar fix with no new UI surface; every behavior change keeps user prose visible instead of silently deleting it. [UX-REVIEWED] f0f109b |
Design Review (Fable 5) — ✅ PASSDesign-level review of Test execution wasn't permitted in this environment, but the diff and description are fully consistent, the new suites are present in the diff, and the design analysis is complete. Design-Verdict: PASS Root-cause grammar fix with a benign failure direction, the downstream deletion hole it exposes closed in the same commit, and both surfaces moved together. Suggestions
[DESIGN-REVIEWED] f0f109b |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All the author's claims check out against the repository: First-Principles-Verdict: PASS A filed defect (#9284) fixed at the grammar that caused it, in both spellings at once, with every narrowing cost enumerated and pinned. What this change shipsIntent: stop the
No undeclared items; the description and diff describe the same job. The rule reuses the discriminator Watch
[FIRST-PRINCIPLES-REVIEWED] f0f109b |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've independently verified the load-bearing claims of this PR rather than inheriting the discovery pass's conclusion. ReDoS / linearity (the security-relevant property): The body is Data-loss direction: New body accepts closers in a strict subset of positions vs the old Parity: The TS I could not ground any (a)/(b)/(c) defect on the changed lines at 80+ confidence. No findings. [OPUS-REVIEWED] f0f109b Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Problem / Motivation
The
[OPTIONS:]label body admits]unconditionally, so it runs to the last closer in range rather than the first plausible one. A turn whose final line mentions a bracket after the marker matches across both:UselabelsThe trailing sentence is gone from the message and is offered to the user as a button. Under
OPTIONS_RE_TRAILER(DOTALL,\Z-anchored, serving the Discord/Telegram/WeCom renderers) the body crosses blank lines too, so the whole closing paragraph goes with it — and with no leading prose the visible text becomes the empty string:→ visible text
Here are your choices.\n\n,labels=' A | B]\n\nRemember to read docs[1'.Filed as #9284, with both classes measured against
origin/mainat56f67aa43.Why it matters
Every consumer treats a match as removable:
parseOptionsreplaces it,slack/format.pyandmessaging/renderer.pycut the visible text atmatch.start(), andwhatsapp/turn_renderer.pypersists the cut turn. So this is silent data loss in the user-facing message, not a rendering glitch — and the swallowed prose is simultaneously surfaced as a clickable option, which is its own confusion.It fires on ordinary agent output. A turn ending in a file path, an array index, a type annotation, or a footnote marker is common, and the marker rule puts
[OPTIONS:]on that same final line by design. It is also backend- and channel-independent, because this is the shared grammar.What changed (motivation → approach → change)
Symptom → a sentence (or a whole final paragraph) disappears from the message and reappears as a pill label.
Root cause → the label body
(?:[^[\n]|\[(?!OPTIONS:))*.[^[\n]includes], so a closer inside the body is free, and the greedy body therefore extends to the last closer that still satisfies the end anchor.The obvious rule is wrong, and the tests say so
A label may legitimately carry a closer, so the body cannot simply exclude
]. The natural fix is to admit one only where the label list continues — where a separator or another closer follows. That is also the rule the streaming probe already applies (CONTINUES_LABELS_RE), so it looked like the grammar and the probe merely disagreeing.Applying it broke two existing tests:
[OPTIONS: Fix [x] logging | Skip]is a first-class supported shape, and both tests say so in their comments ("a literal]inside an option must not truncate the pill"). There the closer is followed by an ordinary word, so continuation alone rejects it. Matching alone is equally wrong in the other direction:[OPTIONS: Alpha ] | Bravo ]]carries closers with no opener at all, and is also tested.So the rule is the union of the two, and each of the three shapes is now pinned:
[OPTIONS: Fix [x] logging | Skip][OPTIONS: Alpha ] | Bravo ]]Use [OPTIONS: A | B] then check arr[0]The pair form is one level deep and its interior excludes
[and every closer (not just ASCII]), so a lookalike codepoint cannot be swallowed into the interior and escape the rule.Disjointness is the linearity argument, not a side note
Two alternatives that both begin at
[would be a genuine ReDoS shape if they could consume the same span — N blocks with two parses each is 2ⁿ on failure. They cannot: the pair form requires that its closer not be followed by a separator or another closer, which is exactly when the continuation form applies. The two lookaheads are each other's negation, so the four body alternatives are mutually exclusive at every position, an unmatched[is left to the bare-[form, and the negated class excludes both[and every closer. There is never more than one way to consume a character.That is asserted rather than argued: six block shapes that each look like both alternatives (
[x],[x] |,[a[b],[a] ]a,[x,[]), repeated 20,000 times and followed by a tail that fails the whole match, so the engine must exhaust every combination it believes exists. Worst single search across the battery is 20 ms. Each lookahead is also entered only at a bracket and bounded by the run it scans.Declining must not delete — the part that made a latent hole load-bearing
Narrowing a grammar whose consumers cut at
match.start()moves the risk downstream: a shape that no longer parses has to survive the partial-marker path instead. It did not.messaging/renderer.py'shide_partialgate asks "could this tail still be a marker in flight?" and answered by looking for ASCII]alone. A marker whose only closers are lookalikes is now declined, falls through to that gate, holds no ASCII]— and so read as in-flight and was cut:Reached via
WeComRenderer.text()→_render_options_as_text, and via_extract_optionsin the Discord and Telegram renderers, which then doself._buf = [body]. That is WeCom's sealed frame and its persisted history entry, plus the sealed Discord/Telegram messages, showing the leading prose with the entire option list deleted and no pills to recover it from. Unrecoverable, and on exactly the channels that cannot render buttons.The gate now tests the whole
MARKER_CLOSERSset — which is the ruletest_hide_partial_does_not_touch_a_closed_bracket_elsewherealready pinned for ASCII ("[OPTIONS: …]that failed the end anchor is prose, not a live marker"), just spelled over one codepoint. Widening it can only ever keep more text, never cut more. It is a different question fromsplit_trailing_protocol_suffix's probe, which asks whether a tail is complete and must stay ASCII-only (revision7115a04awidened that one and was blocked): presence of a closer is not completeness, but it is conclusive evidence of not-in-flight, and only the latter is asked here.One place the rule was looser than what it replaced
The pair form needs the same
(?!OPTIONS:)guard the bare-[form carries. Without it, it opens on a nested head and pairs it with that head's own closer:matched from the outer head — where the old body matched nothing at all — and rendered a pill labelled
see [OPTIONS: x] below | Skip, a raw protocol marker as button text, echoed back as the user's reply when tapped. The guard restores "no bracket form may consume a[that begins a fresh[OPTIONS:" as an absolute property of the body rather than one that holds only for sibling heads.Accepted cost, enumerated rather than summarised
Every shape the union gives up is a closer that satisfies neither half with ordinary words after it, where the input is genuinely indistinguishable from "marker ended, prose followed on the same line". There is more than one way to be that closer, and all of them parsed before:
[OPTIONS: Fix ]x logging | Skip][at all[OPTIONS: Fix list[dict[str, Any]] now | S][OPTIONS: 【重要】修复 | 跳过】【is not an opener, only[is[OPTIONS: Fix [multi\nline] now | Skip]\neven underDOTALLAll four fail toward a visible marker, not toward deleted prose — which is the whole reason they are affordable, and why the gate fix above is what holds the cost up rather than a separate tidy-up. The two are pinned together in one test. Making them parse means matching brackets to arbitrary depth and over an opener set this grammar does not have, which a regex is the wrong tool for; the cost is bounded by the direction it fails in instead.
Row 3 is tracked for a follow-up rather than left as a bare cost: #9375. It is the one of the four that does not need arbitrary-depth matching — only an opener set paired positionally to
MARKER_CLOSERS, so a closer counts as matched by the bracket it actually closes. Measured on a transcription of this body with that added: the two lookalike-pair shapes parse, a mismatched【…]still declines, and this PR's own trailing-prose fix still holds. Deliberately not folded in here — it widens the grammar in the opposite direction from the narrowing this PR is about, and it wants its own tests and its own review. The other three rows stay costs.Note what the union saves relative to the continuation-only rule:
Fix [x] logging,Fix arr[0] now,dict[str, Any]-style labels, and a stray unmatched[all keep working.Out of scope, pinned so it is not read as a regression introduced here
still cuts to
Done., byte-for-byte as it does onmain.],does continue the label list — by the very rule that makes[OPTIONS: Alpha ], Bravo]legal — so no guard applied at the internal closer can tell the two apart. Resolving it means deciding which of the two shapes loses, which is a separate call with its own cost, not a free extension of this one. A test asserts the current output exactly, so the boundary is legible rather than arguable.Relationship to #9174
#9174 (merged 2026-09-07) fixed the wrapper anchor on these same two regexes — a different root cause on the same lines. Its rules are untouched here and compose with this one: a wrapped marker carrying a continuing closer (
`[OPTIONS: Alpha ] | Bravo]`) still parses, and all seven of its wrapper shapes are re-asserted in this PR's tests as guards. Two of the six same-line over-reach rows in the new tables are the wrapped forms of the bug, which #9174's leading-wrapper path inherited unchanged.The rest of the change
src/kiro_crew/constants.py— the rule applies toOPTIONS_RE_LINEandOPTIONS_RE_TRAILER, with the body spelled once per regex (_MARKER_BODY_LINE/_MARKER_BODY_TRAILER) so the two cannot drift from each other. The one body-shaped pattern deliberately not derived from them is_OPTIONS_TAIL_PREFIX_RE, a prefix closure that has to stay looser: a prefix of a legal body need not be a legal body, and[OPTIONS: A ]mid-stream becomes legal only once| B]arrives, so a probe spelled as the real body would call that tail dead and publish the marker as raw text. Its docstring now says that, instead of claiming to hold the grammar's body.src/kiro_crew/messaging/renderer.py— thehide_partialviability gate, widened from ASCII]toMARKER_CLOSERS(see above).website/src/app-sdk/protocol/optionMarker.ts— the same rule in both branches ofOPTION_MARKER_RE, in one commit with the backend so the grammars cannot drift.MARKER_CLOSERSwidening must not skip);stripPartialOptionMarker's "cannot actually reach here", which this change makes reachable (harmless — that branch is behind theisStreaminggate, so the frame is transient); andtest_options_marker_closers.py's "ends at the LAST closer that ends the line". Two test comments claimed[OPTIONS: Fix arr[0] | Skip]is "admitted by both halves at once"; it is admitted by the continuation half only, since the pair half's own lookahead fires precisely because a|follows — which is the disjointness, and means neither alternative is redundant.Tests
test/test_options_marker_label_closers.py(new, 29 cases) andwebsite/src/test/optionsMarkerLabelClosers.test.ts(new, 25 cases), as a sibling to #9174'soptionsMarkerWrapper.test.ts.Both assert the two claims separately throughout, because they are different and only the second is what the user experiences: that the grammar does not match, and that the visible text is unchanged.
Verified as real discriminators rather than restatements, by running every case table against a verbatim transcription of
origin/main's two regexes: of 49 distinct case/grammar pairs, 18 disagree withmain(the eight bug rows — six same-line, two TRAILER paragraph; the eight accepted-cost rows, which parsed before and now do not; the nested head; and one linearity shape that was itself an over-reach) and 31 agree — the guards, i.e. the behaviour the rule had to leave alone. A table where every row disagreed would only be proving that the code changed. The disjointness battery is excluded from that tally on purpose: it asserts elapsed time, not a parse, so "doesmainagree?" is not defined for it.Covered:
Fix [x] loggingspellings), and a separator or another closer keeping an unmatched closer inside the label, with both|and,;MARKER_CLOSERS, not just ASCII, so a model that substitutes one codepoint mid-label is treated identically;[in a label still parses;split_options_trailer— the declined marker must not be silently deleted by the consumer either, which is the assertion that fails without the gate fix;***), the two rules composing,[OPTION:]staying single-select, prose emphasis before the marker still never eaten, the](OPTIONS)tic still composing, same-line prose after the marker still declined, a label still unable to span a line break on LINE, and a marker that does span newlines still closing TRAILER (the rule must not have re-introduced a line boundary into theDOTALLbody);The ReDoS shape assertion in
AssistantMessage.test.tsxis updated to pin the new four-alternative body character for character, since the disjointness is the property the linearity rests on.All existing marker suites pass otherwise: 226 backend cases across the seven
test/files that exercise the marker grammar directly (test_options_cap_contract,test_options_marker_closers,test_options_marker_label_closers,test_options_marker_wrapper,test_parse_options,test_options_buttons,test_weixin), and 621 frontend cases across the seventeenwebsite/src/testfiles that reference the marker orparseOptions.Manual verification
N/A — unit coverage is sufficient and manual reproduction is not deterministic: the trigger is what the model happens to put on the marker's line, not an input a user can type. Both classes are exercised at the parser level on both surfaces, which is the only code the marker passes through.
Related Issues
Fixes #9284
Related: #9110 / #9174 — the wrapper anchor on these same two regexes, different root cause, landed first. This PR is additive to it.
Follow-up: #9375 — the lookalike-pair row of the accepted-cost table above, deferred deliberately.
Overlaps #6823 heavily — please tell me if this should live there instead. #6823 (
feat(chat): zero-turn option actions + visible-only note mode) restructures this same block: it introduces_MARKER_BODY_LINE/_MARKER_BODY_TRAILERunder the same two names and recomposesOPTIONS_RE_LINE/OPTIONS_RE_TRAILERfrom them. Its body is(?P<labels>(?:[^[\n]|{_TEMPER})*)—[^[\n]still admits], so #6823 carries this bug forward into[OPTION-ACTIONS:]as well as[OPTIONS:].That is now confirmed by execution, not read off the diff. Running all three bodies side by side on the two PRs' respective shapes:
The two fix different halves of one defect and neither subsumes the other: #6823 fixes what may follow or be crossed (a shared multi-head temper, plus
_MARKER_LINE_ENDletting a sibling marker terminate the line form), and this PR fixes where the body ends. #6823's own description scopes the remainder out explicitly — "Trailing PROSE still does not terminate a marker -- only a sibling marker does" — so the gap is deliberate there rather than missed. They compose: this body with its(?!OPTIONS:)widened to #6823's_MARKER_HEAD_ALT, plus that terminator, passes every shape from both PRs. Raised on the #6823 thread with the measurements, so the sequencing decision has the evidence attached.The overlap is a rewrite, not a brush: measured as churn against current file size, #6823 is 88% on
src/kiro_crew/constants.py(+680/-24 over 796 lines) and 139% onwebsite/src/app-sdk/protocol/optionMarker.ts(+363/-43 over 291 lines), and it also touchesmessaging/renderer.pyandAssistantMessage.test.tsx. #7157 (render the (recommended) option marker as a badge) adds +125 toconstants.pyand +14/-7 torenderer.py. Both are open, non-draft and active.Whichever lands second has to re-apply the union rule inside the other's composition. This PR is the smaller of the two by more than an order of magnitude (7 files / +846 vs 121 files / +16206), so it is the cheaper one to rebase, and #6823 has to carry the rule regardless because it reintroduces the same body. That is the reason it is proposed standalone — but the call is the maintainer's, and closing this in favour of folding the rule into #6823 is a reasonable answer.
Pattern harvest
Rule candidate: review-prompt
Pattern: when a regex both parses a marker and defines what gets deleted, a greedy body that admits its own terminator does not merely mis-parse — it silently removes the prose between the real terminator and the last one in range. The tell is a body spelled as a negated class that forgets to exclude the closer (
[^[\n]includes]), combined with consumers that treatmatch.start()/replaceas "safe to cut". General rules: for any parse-and-strip grammar, ask what the longest possible match is, not whether the intended one matches, and assert the negative cases as "the text is unchanged" rather than only as "no match" — the two are different claims and only the first is what the user sees. Where a marker's terminator may legitimately appear inside its payload, expect the discriminator to be a union and let the existing test suite tell you so: the single obvious rule here (reuse the streaming probe'sCONTINUES_LABELS_RE) was clean, well-motivated, and broke two tests that pinned the opposite shape — run the whole suite before believing a one-line grammar rule, not just the new tests. When a fix does end up with two alternatives that can begin at the same character, make them each other's negation on some lookahead so no span has two parses; that turns the ReDoS question into a property you can pin in a test instead of an argument in a comment. Finally, check whether the greedy variant is under aDOTALLanchor too, where the blast radius grows from a line to a paragraph.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)