Skip to content

feat(chat): render the (recommended) option marker as a badge - #7157

Open
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:feat/recommended-option-badge
Open

feat(chat): render the (recommended) option marker as a badge#7157
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:feat/recommended-option-badge

Conversation

@rnoack1

@rnoack1 rnoack1 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

This PR introduces a convention and the rendering it needs: an agent marks its
recommended choice by writing (recommended) at the START of an option label. Nothing in
the base repo asked for that marker, so the producer rule below is part of this change
rather than behaviour already in the field — see "Backend" under What changed.

Once the marker exists, two things are wrong with leaving it in the label.

The first is dispatch: the label doubles as the user's next message. Clicking
(recommended) Merge it now sends exactly that, so the user appears to be recommending
something to the assistant. Worse, stripping it naively is its own hazard — a cleaned
label can open with /, @ or [, or BE a sigil-less channel command, and then a click
dispatches something the agent authored rather than words the user chose. That is the
harm the guards below exist for, and it is the one the feature is bought for.

The second is presentation: the marker is plain label text, styled identically to the
instruction and competing with it for a single clamped line's width budget. A badge is
one word, outside the truncating span, and visually distinct — so the recommendation is
legible at a glance instead of being read out of a sentence.

Scope note, stated plainly because an earlier revision of this description claimed more:
the marker is only admitted at the START of a label, and a leading marker is never what
an ellipsis reaches. So this change does not rescue a marker from being clipped away;
that only happened for a trailing marker, a shape nothing emits and which this PR
deliberately does not recognise. ChipBody also already puts the full option on the
chip's title, so the text was never wholly unreachable by hover.

Why it matters

A recommendation nobody can read is worse than no recommendation, because the
turn was written on the assumption it would be visible — the prose above defers
to "the recommended option" that the buttons never name. The failure is also
silent from the authoring side: the marker IS in the response text, so any check
that greps the text passes while the rendered menu shows nothing.

Shortening labels does not fix it. Truncation depends on rendered width, font and
the chip's own box, so any character budget leaves a band of labels visibly cut —
the same reasoning already recorded above chipTooltip for why a 60-char
threshold was the wrong proxy.

What changed

(recommended) is now treated as protocol rather than label text.

  • New protocol/recommendation.tssplitRecommendation(option) returns the
    label with the marker removed plus a boolean saying whether it carried one. The grammar is
    deliberately narrow (recommended and nothing else — the recommended first /
    recommended then ordering variants are NOT admitted, since nothing in the repo emits them): admitting a marker
    paints a badge, so an open-ended parenthetical rule would let (destructive) or
    (see below) style themselves as a recommendation. A label that is only the
    marker is left alone — a badge names no action, and stripping it would send an
    empty message. A label that IS or WOULD BECOME a dispatched command is also left
    alone: the label is dispatched verbatim, and the dashboard treats three leading
    characters as more than plain text — a leading-slash first word is forwarded to the
    harness, a leading @ is resolved by _expand_prompt_mention to a stored
    prompt whose CONTENT replaces the message, and a leading [ opens a reserved
    provenance prefix the backend byte-matches. Stripping a front marker would
    promote inert text into something that runs — (recommended) /clear would
    leave as /clear and erase the transcript, and (recommended) @deploy would
    leave as @deploy and execute that prompt. For such a label the splitter is a
    no-op returning the original text and no marker, so it behaves exactly as if
    the feature were absent and introduces no dispatch path that did not already
    exist. The test is prefix-only; either character inside a label is ordinary prose.
  • parseOptions applies the split, so options are clean at the protocol
    boundary and the four TypeScript hosts send the instruction alone. The messaging
    channels are covered too: split_options_trailer
    (src/kiro_crew/messaging/renderer.py), which 6 renderer modules import, now
    runs strip_recommended_marker over every choice, so a marked label is neither
    displayed nor dispatched verbatim on any channel. That strip carries the SAME
    dispatch guard as the frontend for every form a channel can dispatch — the / and
    @ sigils, the action:: dispatch protocol, the reserved provenance openers, the
    sigil-less channel commands and the plan actions — because a channel
    sends the label as the user's message and an unguarded strip would turn
    (recommended) /clear into /clear. The frontend carries ONE arm the backend does not,
    deliberately: a leading orchestration stop word (stop/cancel/abort), matched on
    the FIRST word rather than the whole label because the dashboard chat endpoint reads
    it that way, so (recommended) stop after this stage would otherwise strip and
    dispatch as a cancellation of a live run. That consumer is reachable only from the
    dashboard, so mirroring the arm onto the channel guard would reserve a word no
    channel dispatches. A channel has no badge to render and is not
    asked for the marker at all, so the strip there is purely defensive, keeping the
    marker carried instead of dropping it. parseOptions also returns
    recommended: the one marked label, or null. A single label rather than a set
    because the only sanctioned producer marks at most one option, and rather than a
    widened options element type because options must stay a string[] — it is
    the canonical string a click dispatches and every picked set is keyed on, so a
    label that differed between display and dispatch would break selection tracking.
  • FollowUpBar renders the marker as a badge in a new ChipBody, as a
    shrink-0 sibling of the clamped span rather than inside it. Position is the
    entire fix. It stays on the same line, so chips remain the same height — the
    constraint ChipLabel's own comment exists to protect.
  • followUpRecommended threaded through the four hosts that render the bar
    (ChatPage, ChatPane, ChatEmbed, SideChat) plus ChatInput. All four,
    not a subset: the marker is stripped for everyone, so a surface left unwired
    would lose the recommendation entirely rather than merely render it plainly.
  • The badge word is held in ChipBadge rather than threaded through the protocol:
    the grammar admits one spelling, so there is no second word to carry. It renders
    through i18nT('components.followUpBar.recommended'), and that key is added to
    all 13 locale catalogs because the badge is UI copy the user reads and the repo's
    catalog-parity gate requires every key in every catalog. The (recommended)
    marker itself is NOT translated — it is protocol vocabulary, matched
    case-insensitively against the literal English word, so translating it would stop
    the parser recognising what an agent wrote.
  • Backend, and this is the producer half of the feature: src/kiro_crew/context.py
    gains _OPTIONS_RECOMMENDED_RULE, which instructs the agent to mark one option.

    Before this PR nothing in the repo asked an agent to emit (recommended), so this
    rule — not pre-existing behaviour — is what makes the marker systematic. It changes
    the critical-rules block of every dashboard session. It is added to the DASHBOARD
    variant only: _CRITICAL_RULES_TAIL is shared by both surfaces, so appending there
    would ask channel agents for a marker no channel can render as a badge — the strip
    above removes it, so the instruction would buy nothing while adding a way to get it
    wrong. Splitting the [END CRITICAL RULES] terminator into
    _CRITICAL_RULES_END is what makes a surface-specific slot possible, mirroring the
    existing _DIFF_RULE_DASHBOARD / _DIFF_RULE_CHANNEL pair.
  • No retraction is shipped, because the selector already re-reads the surface.
    _critical_rules_for picks the block per turn from the LIVE runtime source, not once at
    session start, so a channel turn simply never receives the marker instruction and has
    nothing to retract. An earlier revision did carry an explicit retraction; it was removed
    once the selector made its premise false, and
    TestNoSurfaceRetractsTheMarkerInstruction now pins that absence on every surface.
  • The public SDK surface gains exactly one field, ParsedOptions.recommended.
    splitRecommendation is deliberately
    NOT exported from protocol/index.ts, app-sdk/index.ts or
    public/vendor/kirocrew-app-sdk.mjs: every consumer imports
    ./recommendation directly, so a barrel export would be a zero-consumer
    one-way-door addition riding along in a fix. Apps already get the behaviour
    through the exported parseOptions, whose recommended field subsumes it.
    docs/app-kit/api-reference.md documents that FIELD for out-of-tree apps
    and explicitly withholds the marker's wire spelling and placement from the
    published contract, so apps bind to recommended rather than to the grammar.

A SECOND, INDEPENDENT change ships in the same commit: Slack option clicks stop being
command-interpreted.
This is not part of the marker feature above and is not scoped to
marked labels — it changes behaviour for EVERY Slack option click, including options that
carry no (recommended) marker at all. It is declared separately here because the repo
enforces one commit per PR, so it could not be landed as its own change without splitting
this PR — a scope call left open below.

  • Slack option clicks no longer interpret a leading command token.
    handle_message (src/kiro_crew/slack/handler.py) takes a new
    interpret_commands keyword, True by default so a human-typed message is
    unaffected, and the three Slack option-dispatch sites
    (src/kiro_crew/slack/interactions.py) pass False. It resolves to one
    interpret_as_command decision every command branch reads, so a new branch
    cannot forget it. This applies to every Slack option click, not only the
    ones carrying a marker, because an option label is model-authored either way.
    The harm it closes is separate from the marker work and predates it: a label
    whose first word is one of the nine bangs at handler.py:155-166 was executed
    on click rather than sent as turn content, so an agent-authored !yolo on
    became /kirocrew yolo on and changed the approval mode. The marker-strip
    guard cannot cover this — ! is not one of the reserved dispatch sigils
    (/ and @), so the guard has nothing to match and would strip such a label
    happily. Stated here as its own change rather than folded into the badge work,
    since reverting the badge would reopen it.

  • The two promotion fences can no longer drift apart. The fence is implemented
    twice by construction — strip_recommended_marker (Python, channel dispatch) and
    splitRecommendation (TypeScript, dashboard dispatch) — because each runtime
    dispatches a clicked label itself. The shared vector fixture pinned the forms that
    exist today, which says nothing about the next one: a slash command added to one
    grammar and forgotten in the other would let a marker strip promote a
    model-authored label into a real command on the surface that was missed, with every
    existing test still green. test/test_promotion_fence_drift.py asserts the
    invariant instead, in both directions. Every reserved list must be either PAIRED
    with its frontend symbol and match it, or declared single-surface with a written
    reason; a list or symbol that is neither fails the completeness test, so a seventh
    one cannot skip the pairing. Verified by mutation in four shapes — a Python entry
    the TS array lacks, an alternation branch it lacks, a brand-new backend list, and a
    brand-new frontend symbol — each of which the gate catches.

  • The marker is instructed on the dashboard ONLY. The dashboard is the surface that paints the badge. No channel producer rule asks for the marker, so a channel label carrying one is model drift rather than protocol, and strip_recommended_marker keeps it out of the dispatched string as a defensive measure rather than as half of a paired feature. An earlier revision added a channel-side producer rule and a Recommended: <label> restatement; both are withdrawn, which also removes the hardcoded English prefix that restatement shipped to every locale.

Tests

  • src/test/recommendation.test.ts (new, 33): the split at the leading and
    trailing edge; that both ordering variants are REJECTED; case-insensitivity; a negative
    control
    that (destructive), (see below) and (recommended by nobody at all) are not recommendations; the marker-only label; a no-regex-state guard;
    and the three regressions this turns on — an EDGE marker never survives into a
    dispatchable label, an INTERIOR one is never removed from it, and stripping
    never promotes a label into a dispatched command, by slash or by @ mention. Plan chips
    (Go, Go All, Cancel) are asserted byte-identical, since
    usePlanActionMutation dispatches on exact label equality.
  • src/test/FollowUpBar.test.tsx: its form is pinned against the existing
    spec-decision badge — trailing mono text rather than a leading pill, matching
    apps/spec-builder/components/SpecStatePanel.tsx so the same word does not render in
    two shapes. Colour is the one deliberate difference: that sibling tints itself with the
    accent, which on a chip is the SELECTION signal, so this badge keeps a neutral text
    colour and the test pins zero accent tokens on an unpicked recommended chip. The badge
    is also asserted to be a
    sibling of the clamped .truncate span and never a descendant, with that span
    carrying only the label. An assertion on the rendered text alone would pass
    either way and prove nothing.
  • test/test_messaging_recommended_marker.py (new): the channel half. A marked label
    is not dispatched verbatim through the shared parse, an interior marker survives as
    prose, and a label that would become a slash command, a mention or reserved
    provenance is left exactly as it arrived. Mutation-verified in both directions:
    removing the strip and removing its dispatch guard each fail five cases.
  • test/test_slack_option_click_not_a_command.py (new): every top-level command branch
    in the Slack handler tests the flag, and both option-click dispatches pass it as
    False. Mutation-verified: ungating the ! family fails 4, dropping the flag from one
    dispatch fails 1, and removing all of it fails 7.
  • test/test_slack_linked_option_routing.py (new): a command-shaped label clicked as an
    option lands in the linked slot's queue rather than falling through, and a user-typed
    bang still falls through to the command handler. Mutation-verified: ungating the
    fallthrough fails 5.
  • test/test_dispatch_sigil_parity.py (new): fails if the /, @, [ deny list
    diverges between the backend tuple and the frontend list, so a fourth dispatch
    prefix cannot be added to one surface alone. The same suite pins the action:: dispatch protocol, which the single-character sigil tuple cannot express, against the action router's own spelling, and asserts the frontend guard actually consults the mirrored list rather than merely declaring it. It also asserts the extraction found a
    non-empty list, since an unreadable list would make the comparison vacuous.
  • test/test_context_critical_rules_assembly.py (new): pins the backend producer
    half — that the dashboard block instructs the marker and the channel block does
    not, that the LIVE surface rather than the session's origin decides which block a
    turn receives, that NO surface retracts the instruction, and that every reserved
    dispatch prefix the backend byte-matches opens with [, which is the property the
    frontend guard relies on. Each arm is mutation-verified: re-adding a retraction
    paragraph fails the absence test, and unbracketing one backend prefix fails the
    prefix test. A positive control asserts the scan reaches a real channel turn, and a
    guard asserts the channel's own diff rule was not removed alongside the retraction.
  • capture/followup-recommended-badge.{html,tsx} +
    scripts/capture-followup-recommended-badge.mjs (new): real-layout
    measurement, because the DOM contract above is necessary and not sufficient — a
    badge outside the span but pushed past the chip's edge satisfies every class
    assertion and is still invisible. __measure() takes a Range over the marker's
    own characters and compares its rect against the chip's painted box. fix=off
    renders upstream's shape verbatim, so the before arm is asserted to reproduce
    rather than assumed. 12 of 12 assertions pass across both themes.
  • Full suite 1683/1683 files, 26613 tests. tsc -b clean. eslint on the
    changed files: 0 errors, and the 22 pre-existing hook warnings in
    ChatPage/SideChat are unchanged from main (measured both ways).
  • Rebased onto main; git patch-id --stable identical before and after.

The harness earned its place on its first run: it failed, because the initial
measurement tested horizontal overflow. line-clamp-1 is
display:-webkit-box + -webkit-box-orient:vertical, so an over-long label
wraps and later lines are clipped — the marker disappears downward, and a
horizontal test reports it visible. The paint test now checks both axes, so it is
correct under either clipping mechanism.

It caught a second thing later, which is why the framing above is what it is. When the
fixture was moved to the sanctioned leading marker, the old fix=off assertion — "marker
NOT painted" — went red: markerVisible=true, because a leading marker is never clipped.
The measured defect is therefore not invisibility, and the arm now asserts what is
actually true pre-fix: markerInLabel is true, i.e. the marker is part of the label a
click dispatches. The fix=on arm asserts the converse plus the badge.

Screenshots

Before — the marker sits inside the label, styled as instruction text and part of what a
click would send:

marker inline, unbadged

After — one badge outside the clamped span, and the label holds the instruction alone:

badge beside the label

Light theme, both arms

marker inline, unbadged — light

badge beside the label — light

Note on overlap

#7125 is open on the same
ChipLabel line, swapping line-clamp-1 break-words for block truncate. The
two are complementary and this PR does not depend on which lands first: that one
changes how the label is trimmed, this one moves the marker out of the trimmed
element. If it merges first, the badge is unaffected and only this PR's test
selector for the clamped span needs updating — happy to rebase onto it.

Security-relevant behaviour change on Slack, stated explicitly

The channel-side marker strip alone does not cover !-commands, and this is the
part of the diff a reviewer would otherwise have to find for themselves.

Stripping (recommended) cleans the label, but it says nothing about what the cleaned
label is. (recommended) !yolo on strips to !yolo on, and Slack read a clicked
option through the same path as a typed message — so the click ran the command, turning
on process-wide auto-approval. The frontend sigil guard cannot close this: it can only
enumerate prefixes (/, @, [) and ! commands are a separate family reached by a
different branch.

interpret_commands is new on slack/handler.py::handle_message and passed as False by
both option-click callbacks. It fixes a PRE-EXISTING privilege escalation and does not
depend on this feature
: before any marker existed, a model-authored label spelled
!yolo on was already click-executable, because an OPTIONS label is turn content whatever
it looks like. So this half stands on its own, and reverting the badge would reopen the hole
rather than restore a safe state. With the flag off, !yolo, !agent, !compact,
status, sessions and the !temporary / !incognito privacy modifiers are no longer
reachable by clicking a chip. A user who TYPES any of them is unaffected —
the flag defaults to True. This mirrors the Telegram and Discord dispatchers, which
already pass the same flag on their callback paths.

maybe_route_linked_thread takes the flag too. It deliberately lets a bang command fall
through to normal handling so one can still be run inside a linked thread; for a
model-authored label that fallthrough would persist the turn under the dashboard key
while the linked slot's transcript never saw it, so a command-shaped click would resolve
to a different slot than an ordinary message from the same thread. With the flag off the
fallthrough is suppressed and both resolve identically.

Channels strip a drifted marker and restate nothing

Six renderers parse the trailer through messaging.renderer.split_options_trailer, which
strips the marker so no channel displays or dispatches it verbatim, and appends nothing in
its place. That is a decision, not a gap, and two facts drive it. No channel producer rule
asks for the marker — only the dashboard is instructed to emit one, and
_OPTIONS_RECOMMENDED_RULE is wired into the dashboard block alone — so a marker arriving
at this seam is model drift, and restating drift would give it protocol standing. And the
two-value wrapper has no recommendation identity to restate WITH: it drops that identity by
contract, which test/test_recommendation_seam_parity.py polices, so the choice a line
would name is not knowable there. test/test_channel_recommendation_not_restated.py pins
both halves — the marker leaves the dispatched value, and the body gains no line.

Slack is the one surface that CAN name the choice, because it does not use this parse at
all: slack.format.extract_options_with_recommendation carries the identity explicitly and
renders its own *Recommended:* context block, cut at the same checkbox cap the option
itself is cut at.

Cost of moving the marker out of the label

Recorded as a cost statement, not a proposal: the marker's placement is an open design
question and this section only measures what the alternative would remove.

The marker sits inside the label a click dispatches, so the strip needs a guard for every
value that must not be promoted. Moving it to an out-of-band control tag removes the need
for that guard entirely. Measured by AST at this head:

reserved list literals fate if the marker moves out-of-band
dispatch sigils 2 deleted
dispatch protocols (action::) 1 deleted
provenance openers 19 deleted
injected-origin openers 9 deleted
sigil-less text commands 7 deleted
plan actions 3 deleted
total 41 all deleted
parity suite lines fate
test/test_dispatch_sigil_parity.py 133 deleted
test/test_injected_provenance_parity.py 53 deleted
test/test_reserved_text_commands_parity.py 95 deleted
test/test_reserved_plan_actions_parity.py 92 deleted
test/test_reserved_provenance_parity.py 75 deleted
test/test_dispatch_prefix_declared.py 69 kept — added by this change; guards the dashboard's own dispatch prefixes, which outlive the marker grammar

So 4 of 5 suites and 345 lines go, along with the frontend mirrors of all four lists. The
badge itself survives the move: keepVisibleMarker.ts is the in-repo precedent for a
control tag that renders as nothing, and strip_control_comments already removes such tags
from every plain-text projection. The remaining cost is that label-to-tag correspondence becomes a validation concern
rather than a structural one. The grammar is NO LONGER published SDK surface: this PR
documents only the recommended field and states that the marker's spelling and
placement are not part of the published contract, so moving out-of-band later needs no
app-author migration and is no longer a one-way door.

Why the Slack interpretation flag stays in this change

Two reviews disagree about scope, so both positions are recorded rather than settled here.
One asks for the interpret_commands=False work to land as its own PR. The other holds that
it is load-bearing in this change specifically: ! is not a reserved dispatch sigil, so
without the flag the strip would turn (recommended) !compact into an executed Slack
command. That second reading is why it is not split out. The scope call is open.

Why the channel seam restates nothing, given two lanes asked for opposite things

Worth stating plainly, because the two remedies for the doc/code mismatch pull apart. One
asks for the append to be implemented; the other asks for the channel strip to be deleted
entirely and a drifted marker left as inert label text. Both would cure the mismatch. The
shipped shape is the middle one, and it is chosen from what the tree already asserts rather
than from taste: the strip stays, because a marker must not reach a dispatched value, and
nothing is appended, because the seam that would append has no recommendation identity —
test_recommendation_seam_parity.py states that the two-value wrapper drops it by contract,
so an append there could only name a choice it cannot know is the right one. Implementing it
would mean either widening that seam's signature or guessing, and it would restate a steer
no channel producer rule ever asked for.

The narrower reading also matters: with the channel rule instructing no marker, the code
path exists only to contain drift, so documenting it as containment is the honest
description. This is the reasoning, not a rebuttal — the mismatch is fixed in the tree, in
docs/system-specs/modules/messaging.md, in this section, and in a test that fails if a
restatement is ever appended.

The two-value extract_options is deleted

A review asked whether slack/format.py::extract_options was dead. An earlier revision of
this description argued it was live and should be kept; that was wrong about the outcome, and
this diff DELETES it. Its one real consumer discards the choices it returns, so that call site
now uses the three-value extract_options_with_recommendation directly
(subagent.py:101 imports it, subagent_manager/run.py:1634 calls it). Only one spelling
survives, and two ratchets pin that: test_recommendation_seam_parity.py:80 asserts no
def extract_options( exists in the module, and test_slack_mirror_recommendation.py:186
asserts the attribute is absent, so a re-add fails the suite rather than passing silently.

@rnoack1
rnoack1 requested a review from a team August 31, 2026 00:58
@rnoack1
rnoack1 requested a review from a team as a code owner August 31, 2026 00:58
@rnoack1
rnoack1 requested a review from krishdhasmana August 31, 2026 00:58
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 8d5dc7e95c553d637f6f026fa46cee9b4e992ed1 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 8d5dc7e

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of 8d5dc7e95c553d637f6f026fa46cee9b4e992ed1 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Reviewed the full patch: FollowUpBar badge (ChipBody/ChipBadge), splitRecommendation guards, the 13 locale keys, the Slack *Recommended:* context block, and the Slack interpret_commands change. No blind read exists in this lane and the PR's screenshots live only on the fork, so the user-visible surfaces are evidence gaps. Final review:

UX-Verdict: CONCERNS

Sound badge design and honest dispatch, but every user-visible surface — chip badge, Slack Recommended line, raw-marker fallback — has been seen by no cold reader.

Watch

  • Guarded labels resurface the exact harm this PR fixes: splitRecommendation declines any label whose first word is stop/cancel/abort (unconditionally, live run or not), so a drifted (recommended) Cancel the deploy draws the literal marker in the chip and a click sends it as the user's own words, surfaced only by a console.warn. Low frequency (producer drift) × medium impact (protocol litter + garbled sent message) × persists per menu; the producer rule is the sole mitigation — a human should confirm that's acceptable.

Evidence gaps

  • Dashboard recommended chip badge (unpicked, picked, dark, light) — screenshots exist only as fork raw-URLs in the PR body; push the branch or commit temp-screenshots/followup-recommended-badge/* so a blind read can run.
  • Slack *Recommended:* <label> context line, both variants (plain, and with the "— reply with that option to choose it" overflow suffix) — no screenshot anywhere.
  • Declined-label state (chip rendering (recommended) /clear verbatim) — no screenshot; the capture harness's arms don't cover it.

Suggestions

  • ChipBadge's separator is aria-hidden, so the button's accessible name flattens to "Merge it now recommended"; add a visually-hidden delimiter (e.g. , ) in ChipBody so screen readers don't glue the badge onto the instruction.

[UX-REVIEWED] 8d5dc7e

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 8d5dc7e95c553d637f6f026fa46cee9b4e992ed1 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 8d5dc7e

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 8d5dc7e95c553d637f6f026fa46cee9b4e992ed1 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

In-band marker placement forces the dispatch grammar to live in three copies — TS guard, Python guard, agent prompt — held together only by parity tests.

Watch

  • The dispatched label doubles as protocol, so every reserved form (41 literals across sigils, action::, provenance openers, plan actions, stop words) is now mirrored in recommendation.ts and constants.py and restated in prose to the model via _OPTIONS_RECOMMENDED_RULE; the PR's own cost table shows an out-of-band control tag (existing precedent: keepVisibleMarker / strip_control_comments) deletes the whole class, and the description leaves placement "an open design question." Shipping this shape means every future dispatch prefix anywhere in the system must also be registered in the fence, forever, or a stripped marker promotes a label into a command on the surface that forgot.
    Clears when: a maintainer explicitly ratifies in-band as the permanent shape, or a follow-up issue commits to the out-of-band move the unpublished grammar was designed to permit.
  • When the model marks a guarded label anyway (a plan chip, a stop … sentence, a /-command), the feature degrades to exactly the two harms it exists to prevent — raw (recommended) drawn on the chip and dispatched as the user's words — surfaced only by a console.warn. The prompt rule is the sole real defense, and prompt compliance is probabilistic.
    Clears when: the guarded-refusal path gets a user-visible or telemetry-visible signal, or the out-of-band move makes the case unrepresentable.

Suggestions

  • The Slack interpret_commands half is correctly kept (it closes the !-family hole the sigil guard cannot see and aligns Slack with the four transports that already carry the flag); record that scope decision as settled in the PR rather than "open," so the next reviewer doesn't relitigate it.

[DESIGN-REVIEWED] 8d5dc7e

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 8d5dc7e95c553d637f6f026fa46cee9b4e992ed1 via the fork AI-review pipeline — 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.

I have everything I need. Final check of my counts: strip_recommended_marker runtime consumers — slack/format.py only (rest are tests); interpret_commands siblings on teams/discord/telegram — verified in base; capture-harness convention — verified; 13 locale files — counted in the diff. Emitting the review.

First-Principles-Verdict: CONCERNS

The description claims a channel-wide marker strip the diff deliberately removed — and the guard apparatus contains a hazard this PR's own in-label placement creates.

Not justified as shipped

  • 5 — rides along: real harm (model-authored label executing !yolo/status), but it is a standalone fix for the last unfixed dispatcher, not badge work.
  • 7 — one consumer, generalized: _PLAN_AUTO_RUN_ACTIONS is read only by chat_handlers.py (2 uses, its original home); the "leaf module avoids a cycle" rationale holds for _RESERVED_PLAN_ACTIONS, not this set.

What this change ships

Intent: let the agent visibly steer the user toward one option by marking it (recommended) and rendering that as a badge — an ADDITION; the producer convention originates in this PR.

  1. Dashboard sessions now instruct the agent to mark one recommended option — justified
  2. Dashboard chips draw a "recommended" badge; a click sends the label without the marker — justified
  3. Slack posts mirroring dashboard turns restate the pick as a "Recommended:" line — justified
  4. A marker whose removal would expose /, @, provenance, a plan action, or a stop word stays untouched, drawn and dispatched raw — justified
  5. Slack option clicks no longer execute channel commands or privacy modifiers (interpret_commands=False) — rides along
  6. Non-Slack channels deliberately leave a drifted marker visible as label text — justified
  7. Plan-action/stop-word literals moved into constants.py as the parity anchor — one consumer, generalized (_PLAN_AUTO_RUN_ACTIONS only)
  8. New SDK surface parseOptions().recommended / followUpRecommended for app authors — justified
  9. "recommended" added to all 13 locale catalogs — justified
  10. Isolated capture harness + Playwright measurement for the badge — justified

Watch

  • The description asserts "split_options_trailer … now runs strip_recommended_marker over every choice, so a marked label is neither displayed nor dispatched verbatim on any channel." The diff ships the opposite: messaging/renderer.py changes only a docstring, and new test_messaging_recommended_marker.py opens "A channel does not strip a (recommended) marker; it renders it as label text," recording that the strip and its wecom/weixin command list were removed from an earlier revision. A human merging on the description believes channel coverage that does not exist. Clears when: the description's channel-strip and "sigil-less channel commands" guard claims are corrected to the shipped no-strip decision.
  • The promotion hazard ((recommended) /clear/clear) exists only because the marker rides inside the dispatched label — a placement the diff's own api-reference calls "still being settled," naming out-of-band carriage as the successor. The containment cost is two mirrored guard sets (strip_recommended_marker in constants.py, applyMarkerGuards in recommendation.ts), a third provenance-prefix copy beside the originals in dashboard/state.py, a shared fixture, and ~8 guard/parity test files — all deletable the day the marker leaves the label. Clears when: a named constraint requires in-label carriage (e.g. history/backfill replay), or the marker moves out-of-band and the guards are deleted.

Subtractions

  • Move _PLAN_AUTO_RUN_ACTIONS back into dashboard/chat_handlers.py — its only runtime consumer (2 uses there); no dispatcher outside that file reads it, so the cycle rationale doesn't apply.

[FIRST-PRINCIPLES-REVIEWED] 8d5dc7e

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@rnoack1
rnoack1 force-pushed the feat/recommended-option-badge branch from e4fcbc6 to a440191 Compare August 31, 2026 02:31
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@rnoack1
rnoack1 force-pushed the feat/recommended-option-badge branch from a440191 to a1c3b2d Compare August 31, 2026 02:43
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@rnoack1
rnoack1 force-pushed the feat/recommended-option-badge branch from a1c3b2d to 9b0718c Compare August 31, 2026 03:10
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@rnoack1
rnoack1 force-pushed the feat/recommended-option-badge branch from 9b0718c to e29337c Compare August 31, 2026 04:34
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Sep 1, 2026
@rnoack1
rnoack1 force-pushed the feat/recommended-option-badge branch from 0ed8a41 to 31dbb9a Compare September 1, 2026 01:42
@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: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 1, 2026
@rnoack1
rnoack1 force-pushed the feat/recommended-option-badge branch from 31dbb9a to 29c56c6 Compare September 1, 2026 03:18
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@rnoack1
rnoack1 force-pushed the feat/recommended-option-badge branch from 29c56c6 to df34915 Compare September 1, 2026 04:01
@github-actions github-actions Bot removed the readiness: checking Automated validation is still running label Sep 1, 2026

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is a better design honestly. With limited chips, adding tags does not help much. I would rather we reorder these chips to put the most reasonable stuff at the beginning as always. Will let other designers decide.

@rnoack1

rnoack1 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

I am not sure if this is a better design honestly. With limited chips, adding tags does not help much. I would rather we reorder these chips to put the most reasonable stuff at the beginning as always. Will let other designers decide.

@iamwhatever I added steering in my local environment to say explicitly mark your recommended option(s) , and though the first option is usually recommended, sometimes it is not, and something the last option is "Nothing more (recommended)".

Also sometimes 2 options are (recommended first), (recommended then) so although the intention may be to have the most recommended options in the front, sometimes more options are presented than the agent "recommends". I don't think it necessarily has to be a core feature like this PR, as it can be accomplished with steering - but was trying to figure out if there is a way for everyone / team members to get a similar behavior without needing to add steering.

The in button chip is potentially cool because you could label things other than recommendations potentially with custom logic or hooks, not sure. Maybe another option for just recommendations could be rendering with different color or outline?

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #6823 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6823: MERGE_DISCUSSION. Two open PRs widen the same option-protocol interfaces and the same chip component with different, composable features; they need a merge order rather than a choice. Files: website/src/app-sdk/protocol/options.ts, website/src/components/FollowUpBar.tsx, website/src/components/ChatInput.tsx, website/src/components/ChatPane.tsx, website/src/pages/ChatPage.tsx, website/src/pages/chat/SideChat.tsx, website/src/app-sdk/ChatEmbed.tsx.
  • This PR is OVERLAPPING with PR #7125. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7157: MERGE_DISCUSSION. The merged PR is the trimming mechanism 7157 must work under, not an implementation of it. Already absorbed into 7157's base. Files: website/src/components/FollowUpBar.tsx, src/kiro_crew/context.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@rnoack1

rnoack1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

The chips are capped at min(100%, clamp(18rem, calc(50% - 0.1875rem), 26rem)), i.e. 288–416px, and in the default single-line layout each chip is shrink-0, so the badge changes neither the chip width nor how many options are visible — it only shortens the marked chip's own label. Measured on the committed before/after pair at the same width, the cost is about 14 characters ("in board order" is what gets clipped): 16px of fixed chrome plus the word itself, against a label budget of chip-width minus 26px.

On reordering: worth noting the two are not exclusive — ordering communicates rank, the marker communicates which single option the agent actually recommends, and the agent emits that marker as text today with no way to signal position. Happy to defer to designers on which carries it. One caveat if you compare the screenshots: they were generated before the ordering-variant forms were dropped, so they show a second recommended then badge the current parser no longer produces. I will regenerate them.

@rnoack1

rnoack1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Closing out the screenshot caveat from my previous comment: the regenerated pair is already on the branch, so the stale second badge is gone — my "I will regenerate them" was a beat behind the branch.

Verified at head 453b6bb4206f4637abe7cc640e2a3f167ca8c365:

  • website/capture/followup-recommended-badge.tsx:43-47 — the fixture's option list is three labels with exactly one marker, on line 44. The only other (recommended) in the file is line 2, in its own doc comment.
  • website/scripts/capture-followup-recommended-badge.mjs:60,63 — the harness asserts badgeCount === 0 for the before-state and badgeCount === 1 for the after-state. All 12 assertions pass.
  • The four committed PNGs are byte-identical across the last two rebases, so the images in the diff are what the current parser produces.

The design question you raised is untouched by any of this — badge versus reordering is still yours and the other designers' call, and I have not moved it.

@rnoack1

rnoack1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

One change since you looked, on the "does not help much" point specifically — at head 970b9b82b0a27afd852f4a08388309260970cccb:

The badge previously shared the picked chip's own fill, so once an option was selected the tag melted into it and stopped being readable at exactly the moment it mattered. It now uses a distinct fill: website/src/components/FollowUpBar.tsx:223 renders border border-accent/40 text-accent bg-bg-elevated, against the picked chip body's border-accent/50 text-accent bg-accent-subtle at :158. The four committed capture frames were regenerated from that state.

Nothing else about the badge changed, and I have not touched the ordering question — badge versus reordering is still yours and the other designers' call, and I have deliberately left it open rather than implementing either side.

@rnoack1

rnoack1 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for looking. Measurements at the current head 9c555e8b1 so the design question has the same numbers in front of it — I have not changed the design you are questioning, and I am not trying to talk you out of it.

Your premise is right, and this branch does not touch it. website/src/index.css:2144 caps the chip: .followup-chip{max-width:min(100%,clamp(18rem,calc(50% - 0.1875rem),26rem))}. This branch changes 0 lines of that stylesheet — 23 other files under website/src are in the diff, so that is the cap being left alone rather than me failing to find it.

Where the badge sits relative to the label. website/src/components/FollowUpBar.tsx:234 renders the badge and label as a flex row: the badge at :221 is shrink-0, the label at :195 is block truncate min-w-0, so the label absorbs the truncation and the chip does not grow past that cap. :237 returns the label alone when a chip is not the recommended one, so unmarked chips render exactly as they do today. The badge's width is spent on one chip per list, not on all of them.

On reordering "as always" — there is no ordering behaviour in place to build on yet. website/src/app-sdk/protocol/options.ts:63 is split(sep).map(o => o.trim()).filter(Boolean).map(...), and neither that file nor FollowUpBar.tsx contains any .sort( or .reverse( (210 files under website/src do contain .sort(, so the search does find them when they exist). The agent-facing options rule does not mention order either. So today the chip order is whatever the producer emitted.

That last measurement is the useful part of your suggestion: because nothing sorts, "most reasonable first" would be a change to the producer guidance rather than to this component — which is cheap, and not mutually exclusive with a badge. Ordering could carry the ranking while the badge names the pick explicitly for a screen reader or a user who does not read position as ranking.

Which of the two we want, or both, is your and the other designers' call — I have not implemented the reordering, and I will follow whichever way you land.

@bolichen97

Copy link
Copy Markdown
Collaborator

@rnoack1 the 2026-09-08 open-PR relationship audit found two newer open PRs writing into the same lines as this one. The audit read this PR at 45a69ce; the head has moved since, but every file below is still in the current diff.

#8613 (session_escalate, @CrysisDeu) shares website/src/app-sdk/protocol/options.ts. The goals are unrelated, the collision is mechanical: #8613 adds a new early return in deriveFollowUpOptions for m.role === 'escalation', while this PR makes followUpRecommended a required field of FollowUpDerivation and widens all eight return literals. Once this PR lands, #8613's new literal is missing a required field and fails tsc. This PR owns the interface change, so land it first and ask @CrysisDeu for the one-line addition on rebase.

#8003 (subagent credit usage, @welikoiwanenko) shares src/kiro_crew/subagent.py and src/kiro_crew/subagent_manager/run.py, with no shared behaviour. #8003 inserts format_subagent_usage into _COMPONENT_GLOBAL_BINDINGS two entries below the extract_options line this PR renames, and inserts usage.settle() calls directly above the cleaned, _ = extract_options(result_text) statement this PR rewrites into a three-value form. Both edits sit inside the same hunk window, so whichever lands second re-applies those few lines. No split or scope change is needed on either side.

Neither PR duplicates this one. A note on each about landing order is enough, and @CrysisDeu / @welikoiwanenko may want to watch this PR for the merge order.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

The marker sat inside the chip's single truncated line and the ellipsis reached
it first, so the recommendation was readable only by clicking every chip.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants