fix(dashboard): keep-visible marker exempts mid-turn deliverables from collapse-all - #7960
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound, precedented intent-marker design — but the in-content tag makes every present and future egress leg a silent leak site. Watch
Suggestions
[DESIGN-REVIEWED] 8e630e7 |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS Solid invisible-plumbing fix for buried mid-turn deliverables, but the fence-ambiguity veto leaks the literal control tag onto channel, preview, and speech surfaces. Watch
[UX-REVIEWED] 8e630e7 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence gathered: producers verified ( First-Principles-Verdict: CONCERNS The fix and its backstops all trace to named harms; one depth note — the frontend strip fixes phantom search for only one of three tag families. What this change shipsIntent: stop collapse-all from burying a substantive mid-turn report the user needed (#7948) — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 8e630e7 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
71e0221 to
9612a6d
Compare
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: |
|
Disposition: fixed — cross-surface gap (rule taught in
|
|
Disposition: rebutted — suggestion to replace the marker-specific strip in
|
|
Disposition: fixed — Slack/Discord users would see literal
|
|
Disposition: fixed — subtraction 1 (shrink the rule's scope to the dashboard-only slot; drop the
|
|
Disposition: rebutted — subtraction 2 (retire the marker-specific spelling in
|
9612a6d to
97f0505
Compare
|
Disposition: fixed — the strip-surface sweep was incomplete:
Fixed in On the standing-invariant concern: the rule now applied per surface is "whole-comment strip wherever a fence-protection pass makes it safe (preview, voice); marker-specific strip where fidelity forbids touching fenced content (search, copy)". Channel outbound formatters stay untouched by design: under the dashboard-only prompt rule (round 1) channel agents never learn the marker, and cross-surface relay of dashboard text remains the documented residual boundary rather than a strip site in this PR. The PR body's wrong "two places" claim is corrected to the four-site enumeration. |
|
Disposition: fixed — Copy pasted a literal
Fixed in Your second bullet (speak path): confirmed and covered by the same commit — |
|
Disposition: fixed — the "two strip sites" claim was wrong;
Fixed in |
97f0505 to
b383d8b
Compare
|
span=69c47ee97daa
Fixed in |
|
span=f616f9d8abec
Fixed in |
|
Disposition: accepted-and-deferred — the producer-side-only channel guard is a real residual, tracked in #8005 (label
Mechanism confirmed ( One correction to the suggestion carried into #8005: the strip there must be the recognized-control-tag form, not the "same whole-comment shape as On the second Watch bullet (compliance-dependence in both directions): inherent to the accepted Not marked security/data-loss: the residual is a cosmetic marker line on a non-compliant path, so deferral is permitted under the disposition contract. |
b383d8b to
50e48ff
Compare
|
span=5791404b92d7
Fixed in Opposite-failure-mode check per the widening rule: the post-strip pass applies the same scrubber already applied pre-strip — same false-positive profile; the only delta is strings that become pattern-matching after syntax removal, which is exactly the flagged class. Idempotent (placeholders survive re-scanning). Tests added in The same commit fixes the co-located CodeQL |
fffe160 to
fb3d608
Compare
|
Disposition: fixed — the counted unfixed sibling: the mirror-link backfill leg.
Correct count and correct class — your grep was the enumeration mine missed. Fixed in |
|
Disposition: fixed — dangling doc pointer to the deleted constant.
Fixed in |
|
Disposition: accepted-and-deferred — the unenforced strip invariant; the class-deleting refactor is tracked in #8059 (label
Same class as your round-8 finding, and this round proved the point again in real time: First Principles found a third bypass leg ( |
|
Disposition: fixed — recognizer-pair drift, including the bound divergence you caught, plus your suggested conformance corpus.
Fixed in |
fb3d608 to
a7c21fb
Compare
|
span=f634241daeea
Fixed in |
|
span=38644d4f5240
Fixed in |
|
Disposition: accepted-and-deferred — strip-by-enumeration; the tracking issue you ask for exists: #8059 (label
#8059 carries precisely this design — lift the trailing tag block into message meta at the single ingestion point on the |
|
Disposition: rebutted — the mid-message-emission gap is the deliberate, cheaper side of an adjudicated trade-off, kept as-is.
Agreed on the mechanics, and it stays by design: position-independent stripping was tried and retired across rounds 5–8 — GPT falsified it against three quoted-code dialects in succession (inline code, fences, variable-length spans), i.e., treating mid-body tags as control tags DELETES visible content, a worse failure than a stray literal tag. The asymmetry of harms picks the tail contract: a non-compliant mid-body emission renders as nothing on the dashboard (HTML comment) and as a cosmetic literal on plain-text channels, while a mid-body strip corrupts quoted code everywhere. The corpus documents mid-body-as-content as contract. Your "watch real transcripts" suggestion is the right monitor, and #8059's meta-lifting also shrinks this gap structurally: once the marker is parsed at ingestion, a producer-side lint or re-anchor becomes possible at the single point where the message is finalized. |
|
span=f634241daeea
Span history (the sibling series):
The invariant question: the fence walker exists only because control tags live in message CONTENT, so every projection must re-parse markdown after the fact. The class-deleting alternative is already designed and tracked as #8059 (Design has independently converged on it in 3 consecutive rounds): parse the trailing tag block ONCE at message finalization — where the tail is unambiguous and no rendering context exists — store Options — which do you want?
My read: option 3 documents a real (if exotic) data-deletion input, so it needs an explicit human override, not my judgment; option 1 continues a losing series; option 2 is the correct fix but a scope call only you can make. Awaiting your ruling — no further pushes on this span until then. (Held locally, unpushed, unrelated to this span: First Principles round-12 fixes — the Slack backfill egress leg strip and the |
a7c21fb to
6ff7966
Compare
|
span=f634241daeea
The fix (both walkers, Why this closes the span class rather than this instance: the three hits all had the shape "an input where fence-interior content is stripped." The failure modes are asymmetric — wrongly stripping deletes visible content; wrongly not stripping leaves an HTML comment the renderer never shows — so the veto makes the entire class structurally unreachable: any fence-open the exact grammar cannot classify now yields a no-op, never a strip. A further sibling would require the over-approximating detector to miss a fence run entirely, and it matches every fence run not preceded by prose. The residual cost is a documented feature-miss (container-nested-fence messages skip the keep-visible exemption), never content. Pinned by 5 new shared-corpus cases asserted by both suites (list-contained unterminated fence preserved; blockquote fence preserved; list-contained closed fence vetoed — the documented trade; over-indented fence vetoed; container-fence example quoted inside a closed plain fence still strips — the precision bound). #8059 (meta-lifting at ingestion, due 2026-09-23) remains the tracked long-term fix that deletes projection-time recognition entirely. |
Disposition: rebutted (grammar kept; the description clause it flagged is fixed — reworded in the current body).
The code-emitter count is correct — verified: The legitimate half of the finding — the description's "fixes leakage of the heartbeat's routing tags" overclaimed a file-suffix scenario the tail grammar deliberately ignores — is fixed: the body now names |
Disposition: accepted-and-deferred — this is the standing fan-out concern, adjudicated in rounds 8–11 and tracked as #8059 (
Why still deferred rather than pulled in-PR (re-affirmed by the maintainer's round-13 ruling): moving parsing to ingestion changes message persistence and requires a legacy dual-read for existing transcripts — a scope the maintainer weighed against this PR twice and both times routed to the follow-up. This round's conservative-veto change reduces the cost the fan-out carries in the meantime: the duplicated walker no longer needs CommonMark-complete container tracking on either side, because ambiguity is now a structural no-op (strip only when provably outside every fence), so the grammar the ~10 sites share is final rather than still-converging. The prompt-compliance residual ("a turn where the agent forgets reproduces #7948") is the documented no-heuristic trade from round 1: intent signals only, accepted in the issue design. |
…m collapse-all Collapse-all mode shows only a turn's last substantive assistant message (findConclusionIdx) and folds everything earlier into the 'Worked through N steps' pane. A substantive mid-turn deliverable — a report or synthesis followed by a terminal tool call and a short sign-off — has no visibility bypass, so the sign-off becomes the visible conclusion and the deliverable is buried. Add an explicit intent marker, mirroring the [OPTIONS:] hand-back exemption (intent signal, not a size heuristic, per the documented design rule in TurnBlock.tsx): - keepVisibleMarker.ts: canonical <!-- keep-visible --> regex + probe helper, following optionMarker.ts lastIndex-safety conventions. - TurnBlock.tsx: isKeepVisible joins isVisibleInline, so marked messages bypass the collapse pane (shared by ChatPage and app-sdk ChatMessageList, which render through this component). - searchableText.ts: strip the marker so search never phantom-matches inside the invisible comment. - preview_text.py: strip HTML comments from plain-text previews — the markdown pipeline renders them as nothing (rehype-raw comment nodes), so they must not leak into sidebar previews. Also covers the heartbeat's <!-- deliver:... --> routing tags. - context.py: document the marker in the injected critical rules (both dashboard and channel variants) so agents emit it on mid-turn deliverables. The marker only prevents folding — it grants no capability, so untrusted content carrying it can at worst keep its own text visible, which is the default outside collapse mode. Fixes #7948
6ff7966 to
8e630e7
Compare
|
span=1fd39a49230f
Disposition: fixed (in
Applied exactly that, to all four leading-indent classes: Note for the record: |
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
bolichen97
left a comment
There was a problem hiding this comment.
Approving — the diff is the collapse-all exemption fix it claims, with no visual delta.
Verified against the diff:
isKeepVisibleinTurnBlock.tsxjoinsisVisibleInlineexactly like the existingisHandBack[OPTIONS:]exemption;isConclusion(TurnBlock.tsx:60) is a role predicate (assistant/streaming/file), not "the turn's conclusion", so the marker genuinely fires on a mid-turn message rather than only on the last one. No change tofindConclusionIdx,splitSegments, or any rendered element.- Zero pixels added: the marker is an HTML comment, so rehype-raw emits a comment node the react renderer skips. No new component, icon, label, or interaction — consistent with the
<!-- no-visual-delta -->claim. - One grammar, two recognizers:
constants._TRAILING_CONTROL_LINES_REandkeepVisibleMarker.tscarry identical bounds (indent ≤3, whitespace ≤16, body ≤256) and identical fence walkers, pinned by the sharedtest/fixtures/control_tag_corpus.jsonasserted from both suites, so a bound edited on one side goes red on the other. - Fail-safe direction is right:
_in_open_fenceVETOES on an ambiguous container-prefixed/over-indented fence candidate, so ambiguity costs the exemption, never visible content. Unterminated<!--is deliberately unmatched, so no swallow-to-EOF deletion of prose. - Egress is closed, not partially closed: strip-then-redact at
display_safe/display_safe_forplus the four sinks that bypass them (chat_runner._deliver_cross_surface_reply,slack/gateway._deliver_channel_reply,chat_mirror,chat_slack), so a dashboard-authored tag cannot surface as literal text on a channel. voice_reply.strip_markdownre-runsredact_exfiltration_urls+redact_credentialsafter the strips (imports already present at line 41) — correct, since removing an interposed comment can rejoin a split credential that the pre-strip scan missed.- Prompt half lands only in
_DIFF_RULE_DASHBOARDandtest_context.py::TestKeepVisibleMarkerRuleasserts its ABSENCE from the channel variant, so channel sessions are never taught an emitter their renderer would show literally. - Test coverage matches the risk surface: TurnBlock marked-vs-unmarked fold split, preview/voice/display-safe strips, ordinary and unterminated comments preserved, inline-code and fenced tags preserved.
All 36 checks pass (including Design/UX/First-Principles/GPT/Opus reviews and Screenshot Evidence).
Problem / Motivation
With the transcript collapse preference on (
collapseAll— "all working steps collapse, only final assistant text visible"), a substantive mid-turn assistant report folds into the collapsed "Worked through N steps" pane whenever any later assistant message in the turn is also substantive. Observed shape: a monitor-loop campaign close posted a full results synthesis, then calledautonudge_stop, then posted a short "Loop stopped…" sign-off —findConclusionIdxpicked the sign-off as the turn's conclusion and buried the synthesis.Why it matters
Users running collapse-all lose the turn's actual deliverable — the one message they needed — unless they think to expand the steps pane. Every agentic long-turn pattern (monitor cycles, queued-message resumes, injected subagent/workflow completions) produces exactly this shape, and
TurnBlock.tsxalready documents the class ("A single turn can contain SEVERAL hand-backs…") but only exempts[OPTIONS:]-bearing messages.What changed (motivation → approach → change)
Symptom → root cause:
isVisibleInlinehas bypasses for widgets/images,[OPTIONS:]hand-backs, crew replies, error/mcp_oauth rows, workflow/spawn/completion cards, MCP-App rows, and diff cards — but a plain prose deliverable has no bypass, so only the last substantive message survives.Approach: an explicit, invisible intent marker, mirroring the
[OPTIONS:]hand-back exemption. A size/shape heuristic was deliberately NOT used — theisHandBackcomment documents that rejection ("gating on size would override a preference the user set on purpose"); intent markers are the accepted pattern. HTML-comment control tags are an existing convention (heartbeat<!-- deliver:... -->).Change:
website/src/app-sdk/protocol/keepVisibleMarker.ts(new): canonical<!-- keep-visible -->regex +hasKeepVisibleMarkerprobe, followingoptionMarker.tslastIndex-safety conventions.website/src/pages/chat/TurnBlock.tsx:isKeepVisiblejoinsisVisibleInline. Shared by ChatPage and app-sdkChatMessageList(both render through TurnBlock), and applies to interim fan-out turns for free.website/src/utils/searchableText.ts: strip the marker so search never phantom-matches inside the never-rendered comment.src/kiro_crew/preview_text.py: strip recognized control-tag comments from plain-text previews — rehype-raw parses them into comment nodes the react renderer skips, so they render as nothing and must not leak into sidebar previews. Also strips the task planner's<!-- plan_task_id:... -->anchors (task_planner.py:430appends one at the tail) and standalone tail<!-- deliver:... -->lines — the latter have no code emitter, but the shipped prompt (config/prompt.md, heartbeat section) instructs agents to “Route completion with<!-- deliver:dashboard -->tags”, so completion text imitating that instruction is a prompt-induced tail producer; the heartbeat FILE's owndeliver:suffixes are same-line, mid-body content when echoed and are deliberately left alone. The strip is the sharedconstants.strip_control_comments(grammar documented once onconstants.CONTROL_COMMENT_RE): it runs after_FENCE_RE(fenced tags stay placeholder-protected), preserves tags quoted in inline code (rendered literally, so visible), and preserves ordinary and unterminated comments — swallowing to end-of-text on a missing-->would silently delete visible prose.src/kiro_crew/context.py: one rule in the DASHBOARD-only_DIFF_RULE_DASHBOARDblock telling agents to append the marker to substantive reports that are not the turn's final message — without the prompt half, agents never emit the marker and the UI half is dead code. Deliberately NOT in the channel variant: collapse-all is a dashboard-transcript feature (Design/UX/First-Principles round-1 finding, fixed in9612a6d47). The prompt rule contains the EMITTER; the MESSAGE gets a deterministic backstop too (round-6 Design finding): the channel-neutral outbound sinks (messaging.renderer.display_safe/display_safe_for— the dashboard's channel-addressed sends, heartbeatdeliver:routing, and the owner-DM leg) now runstrip_control_commentsfirst, so dashboard-authored text delivered to a channel cannot show end users the literal tag. The shared helper is fence- and inline-code-aware, so a tag QUOTED in code stays visible on channels exactly as it does in dashboard projections.Security note: the marker only prevents folding — it grants no capability. Untrusted content carrying it can at worst keep its own text visible, which is the default outside collapse mode; the marker-neutralization suite passes unchanged.
Tests
website/src/test/TurnBlock.test.tsx: marked mid-turn report stays visible in collapseAll (#7948); unmarked control folds and pins the existing user-preference contract the marker opts out of.test/test_preview_text.py: marker stripped from previews;<!-- deliver:dashboard -->and<!-- plan_task_id:... -->stripped; ordinary and unterminated comments preserved (no swallow); recognized tag quoted in inline code preserved; comment inside a code fence stays placeholder-protected.test/test_context.py::TestKeepVisibleMarkerRule: the marker is documented in the dashboard rules variant and asserted ABSENT from the channel variant.Runs: backend 50 passed (preview + context + full marker-neutralization suite); TurnBlock suites 50 passed / 1 pre-existing expected fail; search suites 260 passed;
tsc -bclean; baselined black / subprocess-encoding / sync-io-in-async / brand / testpaths gates all pass locally on the commit.Manual verification
N/A — unit coverage exercises the exact fold/bypass split (
splitSegmentsassertions on overflow containment), and the marker's invisibility rests on rehype-raw comment-node handling already exercised by the renderer suite.Pattern harvest
Rule candidate: a prompt rule that teaches agents an output marker must ship only in the per-surface prompt variant whose renderer actually consumes that marker (the dashboard vs channel split at
_DIFF_RULE_*) — a marker taught in the shared tail leaks as literal text on every surface whose formatter does not strip it.isVisibleInlineconsumers (collapseAll split and interim fan-out fold sharesplitSegments— one definition, both covered).test/fixtures/control_tag_corpus.json) asserted by BOTH test suites so grammar drift goes red locally: only standalone control-tag lines ENDING the message are control tags, a tail inside an UNTERMINATED fence is visible code (renders literally) and is rejected by both sides, and a fence candidate the exact walker cannot classify (container-prefixed — a fence run after a list bullet, ordered-list marker, or blockquote marker — or over-indented) VETOES the whole decision on both sides: strip nothing, no exemption. The failure modes are asymmetric — wrongly stripping deletes visible fence-interior content while wrongly not stripping leaves an HTML comment the renderer never shows — so the walkers strip only when the tail is PROVABLY outside every fence under an over-approximating candidate detector, and ambiguity may only ever cost the feature, never content, and stacked sibling tags after the marker neither void the exemption nor survive the strip (frontendkeepVisibleMarker.ts; backendconstants._TRAILING_CONTROL_LINES_REviastrip_control_comments). Every producer emits at the tail — the prompt rule says "as its final line", and the task-planner appender emits a newline-prefixed tag, while the heartbeat’sdeliver:tags are HEARTBEAT.md file-format suffixes on checklist lines (not message-tail emissions — echoed into a message they are mid-body content the renderer hides) — so nothing real is missed, and a tag quoted anywhere in the body (inline code, fences, variable-length backtick spans) is structurally untouchable rather than guarded by a code-span grammar (rounds 5–7 each surfaced another dialect the position-independent strip corrupted). Strip sites:searchableText.tsand the Copy button (frontend regex),preview_text.py,voice_reply.strip_markdown(also covers the dashboard Speak button), the channel-neutral sinksdisplay_safe/display_safe_for, and the four direct-egress legs that bypass those sinks (chat_runner._deliver_cross_surface_reply, the Slack proactive-egress chokepoint inslack/gateway.py, thechat_mirror.pylink-backfill leg, and thechat_slack.pythread-backfill leg) — strip-then-redact at every backend site. All quantifiers bounded (CodeQLpy/polynomial-redos).**emphasis) interposed inside a credential splits it, so the pre-strip redaction scan misses it and the strip reconstructs it.strip_markdowntherefore re-runsredact_credentials+redact_exfiltration_urlson its own output — an invariant covering every strip in the function. Idempotent on clean text; also protects thesplit_sentencespath, which had no pre-strip redaction.Screenshots / video
Why no screenshot: the marker is an HTML comment that renders as zero pixels (rehype-raw comment node); the fold-bypass changes visibility only for transcripts carrying the new marker, none of which exist yet, and the exact visible/folded split is pinned by TurnBlock unit tests asserting overflow containment. A seeded-transcript before/after can be produced on request.
Fixes #7948