Skip to content

fix(protocol): keep a label closer only where it is matched or continues the list - #9341

Merged
pepmach merged 1 commit into
mainfrom
fix/options-label-closer-continuation
Sep 8, 2026
Merged

fix(protocol): keep a label closer only where it is matched or continues the list#9341
pepmach merged 1 commit into
mainfrom
fix/options-label-closer-continuation

Conversation

@cixuuz

@cixuuz cixuuz commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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:

Use [OPTIONS: A | B] then check arr[0]
visible text after strip Use
labels `' A

The 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:

Here are your choices.

[OPTIONS: A | B]

Remember to read docs[1]

→ 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/main at 56f67aa43.

Why it matters

Every consumer treats a match as removable: parseOptions replaces it, slack/format.py and messaging/renderer.py cut the visible text at match.start(), and whatsapp/turn_renderer.py persists 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:

test/test_options_buttons.py::TestExtractOptions::test_bracket_inside_option_text_survives
test/test_parse_options.py::test_bracket_inside_option_text
    AssertionError: assert [] == ['Fix [x] logging', 'Skip']

[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:

input admitted by outcome
[OPTIONS: Fix [x] logging | Skip] matched pair parses
[OPTIONS: Alpha ] | Bravo ]] list continues parses
Use [OPTIONS: A | B] then check arr[0] neither declined
_MARKER_LABEL_CONTINUES = rf"(?=[ \t]*[|,]|{_MARKER_CLOSE_CLASS})"
_MARKER_LABEL_PAIR = (
    rf"\[[^[{re.escape(MARKER_CLOSERS)}\n]*{_MARKER_CLOSE_CLASS}"
    rf"(?![ \t]*[|,]|{_MARKER_CLOSE_CLASS})"
)

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's hide_partial gate 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:

请选择:
[OPTIONS: 【重要】修复 | 跳过】     →  ('请选择:', [])

Reached via WeComRenderer.text()_render_options_as_text, and via _extract_options in the Discord and Telegram renderers, which then do self._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_CLOSERS set — which is the rule test_hide_partial_does_not_touch_a_closed_bracket_elsewhere already 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 from split_trailing_protocol_suffix's probe, which asks whether a tail is complete and must stay ASCII-only (revision 7115a04a widened 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:

Note [OPTIONS: see [OPTIONS: x] below | Skip]

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:

shape why neither half admits it
[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 — 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

Done. [OPTIONS: Merge | Wait], details in CHANGELOG[1]     → visible 'Done. '

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 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 to OPTIONS_RE_LINE and OPTIONS_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 — the hide_partial viability gate, widened from ASCII ] to MARKER_CLOSERS (see above).
  • website/src/app-sdk/protocol/optionMarker.ts — the same rule in both branches of OPTION_MARKER_RE, in one commit with the backend so the grammars cannot drift.
  • Comments corrected rather than left to mislead — the module header on both surfaces, which described the pre-change unconditional body; the ReDoS note claiming a closer is readmitted "in exactly one place" (it is two — the pair form's final atom and the continuation lookahead, and the pair form is where the deciding lookahead lives, so it is the one a future MARKER_CLOSERS widening must not skip); stripPartialOptionMarker's "cannot actually reach here", which this change makes reachable (harmless — that branch is behind the isStreaming gate, so the frame is transient); and test_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) and website/src/test/optionsMarkerLabelClosers.test.ts (new, 25 cases), as a sibling to #9174's optionsMarkerWrapper.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 with main (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 "does main agree?" is not defined for it.

Covered:

  • the six same-line over-reach rows and the two TRAILER paragraph rows, each asserted as both "does not match" and "no prose is deleted";
  • both halves of the rule stated positively — a matched pair mid-label (including the two upstream-pinned Fix [x] logging spellings), and a separator or another closer keeping an unmatched closer inside the label, with both | and ,;
  • the continuation half applied to every closer in MARKER_CLOSERS, not just ASCII, so a model that substitutes one codepoint mid-label is treated identically;
  • the pair form is not a requirement — a stray unmatched [ in a label still parses;
  • all four accepted-cost shapes, each asserted as both "does not match" and "the text is unchanged", plus one test that carries the second claim down into 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 nested-head guard, and the out-of-scope separator-tail form, each stated as a decision with its reasoning;
  • not overly narrow: the plain marker on both grammars, all seven of fix(protocol): tolerate a Markdown wrapper around the [OPTIONS:] marker #9174's wrappers (plus ***), 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 the DOTALL body);
  • five linearity guards per surface, including the disjointness battery described above.

The ReDoS shape assertion in AssistantMessage.test.tsx is 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 seventeen website/src/test files that reference the marker or parseOptions.

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_TRAILER under the same two names and recomposes OPTIONS_RE_LINE / OPTIONS_RE_TRAILER from 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:

input: 'Use [OPTIONS: Keep it | Drop it] then check arr[0]'
  base    labels=' Keep it | Drop it] then check arr[0'   surviving text='Use '
  #6823   labels=' Keep it | Drop it] then check arr[0'   surviving text='Use '
  #9341   NO MATCH

input: '[OPTIONS: A | B] [OPTION-ACTIONS: close=Close this tab]'
  base    labels=' A | B] [OPTION-ACTIONS: close=Close this tab'
  #6823   labels=' A | B'    surviving='[OPTION-ACTIONS: close=Close this tab]'
  #9341   NO MATCH

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_END letting 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% on website/src/app-sdk/protocol/optionMarker.ts (+363/-43 over 291 lines), and it also touches messaging/renderer.py and AssistantMessage.test.tsx. #7157 (render the (recommended) option marker as a badge) adds +125 to constants.py and +14/-7 to renderer.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 treat match.start()/replace as "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's CONTINUES_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 a DOTALL anchor too, where the blast radius grows from a line to a paragraph.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A; the rationale lives in the comment block the rule is defined in
  • No secrets, credentials, or internal references in the diff

…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
@cixuuz
cixuuz requested a review from a team September 8, 2026 01:51
@cixuuz
cixuuz requested a review from a team as a code owner September 8, 2026 01:51
@cixuuz
cixuuz requested a review from pepmach September 8, 2026 01:51
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of f0f109b167bae557208d21f4df2f6520629149ce — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The diff is confined to the marker grammar (regexes in constants.py / optionMarker.ts), a partial-marker viability gate in renderer.py, comments, and tests. No user-visible control, label, string, or state is added or changed — the effect is that prose the old grammar silently deleted (and re-surfaced as a pill) now stays in the message, and unparseable edge shapes fail toward a visible marker instead of deleted text. With zero added/changed controls, the empty blind read leaves no evidence gap, and there is no lens-13 transition. The user-facing direction is strictly protective.

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

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of f0f109b167bae557208d21f4df2f6520629149ce — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of f0f109b167bae557208d21f4df2f6520629149ce — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All the author's claims check out against the repository: CONTINUES_LABELS_RE exists at optionMarker.ts:141 as the streaming probe's rule, _OPTIONS_TAIL_PREFIX_RE is real and deliberately looser with its own pinning test, the compiled regex constants are the single backend spelling with five real consumers (preview_text, slack/format, whatsapp/turn_renderer, dashboard/state, messaging/renderer), and the only surviving copy of the old body is a comment in test/test_options_buttons.py:51.

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 ships

Intent: stop the [OPTIONS:] marker from swallowing (and silently deleting) the prose after it — a FIX.

  1. A sentence after a same-line marker no longer vanishes into a pill label — justified (the fix, protocol: [OPTIONS:] label body runs to the LAST closer, deleting the rest of the line (and the final paragraph on TRAILER) #9284)
  2. The final paragraph after a marker no longer vanishes on Discord/Telegram/WeCom — justified (same cause, TRAILER)
  3. The dashboard parser applies the identical rule — justified (single-grammar invariant stated in optionMarker.ts itself)
  4. Messaging hide-partial gate now treats lookalike closers as "not in flight" — justified; the narrowing routes declined markers through this gate, so without it the fix deletes WeCom/Discord/Telegram option lists
  5. Shapes that used to parse (unmatched closer + words, >1-level nesting, lookalike pairs) now show a visible marker — declared, enumerated, fails toward visible text not data loss
  6. A nested [OPTIONS: head can no longer become a pill label — justified (hole the pair form would have opened)

No undeclared items; the description and diff describe the same job. The rule reuses the discriminator CONTINUES_LABELS_RE (optionMarker.ts:141) already applied to streaming rather than adding a second one. No new public surface: the four new _MARKER_* constants are module-private with 1–2 in-module consumers each; grep for the old body [^[\n]|\[(?!OPTIONS:) finds 0 live siblings.

Watch

  • test/test_options_buttons.py:51 still quotes the replaced body (?:[^[\n]|\[(?!OPTIONS:))* in a comment (1 occurrence grepped) — the one stale citation of the grammar this PR retired; the sibling comment in test_options_marker_closers.py was updated, this one was missed.
  • The separator-tail form (Wait], details in CHANGELOG[1]) still deletes prose; declared out of scope with the level stated and pinned by test, so it is deferred, not hidden.

[FIRST-PRINCIPLES-REVIEWED] f0f109b

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of f0f109b167bae557208d21f4df2f6520629149ce and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] f0f109b

False positive or not applicable? A repository writer can comment:
/ai-review override gpt f0f109b167bae557208d21f4df2f6520629149ce: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed f0f109b167bae557208d21f4df2f6520629149ce — this comment is updated in place on each push.

Review details

I'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 (?:PAIR | \[(?!OPTIONS:) | CLOSE(?=sep|CLOSE) | [^[CLOSERS\n])*. I traced the disjointness: _MARKER_LABEL_PAIR's trailing (?![ \t]*[|,]|CLOSE) is the exact negation of _MARKER_LABEL_CONTINUES's (?=[ \t]*[|,]|CLOSE), so any given closer is consumable by at most one of the two closer-bearing alternatives; alt4 excludes both [ and every closer; PAIR's interior [^[CLOSERS\n]* cannot cross a bracket, so from any [ it has exactly one candidate closer (no internal quantifier ambiguity). No span has two full parses → linear. The adversarial shapes ([a[b] , [a] ]a , [x, etc. × 20k with a failing tail) are pinned under 5s.

Data-loss direction: New body accepts closers in a strict subset of positions vs the old [^[\n] (which freely consumed ]), so overall matches never grow — cut spans shrink or the marker declines. The renderer.py widening from "]" not in text[idx:] to not any(c in text[idx:] for c in MARKER_CLOSERS) only makes the hide_partial branch falsy in more cases → keeps more text. Both changes move away from prose deletion, not toward it.

Parity: The TS OPTION_MARKER_RE four-alternative body matches the Python LINE body character-for-character (both exclude \n), and stripPartialOptionMarker's only change is a comment — logic (forming) is untouched.

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 f0f109b167bae557208d21f4df2f6520629149ce.

False positive or not applicable? A repository writer can comment:
/ai-review override fable f0f109b167bae557208d21f4df2f6520629149ce: <one-sentence reason>

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Sep 8, 2026
@pepmach
pepmach enabled auto-merge (squash) September 8, 2026 08:05
@pepmach
pepmach merged commit febf6c6 into main Sep 8, 2026
72 checks passed
@pepmach
pepmach deleted the fix/options-label-closer-continuation branch September 8, 2026 18:53
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

protocol: [OPTIONS:] label body runs to the LAST closer, deleting the rest of the line (and the final paragraph on TRAILER)

3 participants