Skip to content

fix(dashboard): keep-visible marker exempts mid-turn deliverables from collapse-all - #7960

Merged
bolichen97 merged 1 commit into
mainfrom
fix/7948-keep-visible-marker
Sep 4, 2026
Merged

fix(dashboard): keep-visible marker exempts mid-turn deliverables from collapse-all#7960
bolichen97 merged 1 commit into
mainfrom
fix/7948-keep-visible-marker

Conversation

@patrigao

@patrigao patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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 called autonudge_stop, then posted a short "Loop stopped…" sign-off — findConclusionIdx picked 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.tsx already 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: isVisibleInline has 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 — the isHandBack comment 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 + hasKeepVisibleMarker probe, following optionMarker.ts lastIndex-safety conventions.
  • website/src/pages/chat/TurnBlock.tsx: isKeepVisible joins isVisibleInline. Shared by ChatPage and app-sdk ChatMessageList (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:430 appends 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 own deliver: suffixes are same-line, mid-body content when echoed and are deliberately left alone. The strip is the shared constants.strip_control_comments (grammar documented once on constants.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_DASHBOARD block 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 in 9612a6d47). 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, heartbeat deliver: routing, and the owner-DM leg) now run strip_control_comments first, 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 -b clean; 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 (splitSegments assertions 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.

  • Checked the other isVisibleInline consumers (collapseAll split and interim fan-out fold share splitSegments — one definition, both covered).
  • Plain-text projections of assistant content must not resurface the invisible marker — and, symmetrically, strips must never delete VISIBLE content (a tag the assistant quotes in prose or any code dialect). Both recognizers share ONE tail-anchored, case-insensitive, fence-guarded grammar with identical bounded quantifiers (leading indent ≤3 per CommonMark’s indented-code rule; fence closers must be bare per CommonMark 4.5), pinned by a shared conformance corpus (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 (frontend keepVisibleMarker.ts; backend constants._TRAILING_CONTROL_LINES_RE via strip_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’s deliver: 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.ts and the Copy button (frontend regex), preview_text.py, voice_reply.strip_markdown (also covers the dashboard Speak button), the channel-neutral sinks display_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 in slack/gateway.py, the chat_mirror.py link-backfill leg, and the chat_slack.py thread-backfill leg) — strip-then-redact at every backend site. All quantifiers bounded (CodeQL py/polynomial-redos).
  • Removing syntax can REJOIN split secrets: a control tag (or ** emphasis) interposed inside a credential splits it, so the pre-strip redaction scan misses it and the strip reconstructs it. strip_markdown therefore re-runs redact_credentials + redact_exfiltration_urls on its own output — an invariant covering every strip in the function. Idempotent on clean text; also protects the split_sentences path, 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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

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

  • The strip lives at eight call sites because the tag rides in stored content forever; the PR itself had to hunt down "the four direct-egress legs that bypass those sinks" (chat_runner, slack/gateway, chat_mirror, chat_slack). Cause → mechanism → consequence: a future egress leg that calls redact_for_display directly → nothing forces the strip-first step → channel users see literal <!-- keep-visible -->, and nothing goes red. Same failure class as the fence-walker's "rounds 5–8 each surfaced another dialect".
  • Efficacy and containment are both prompt-bound: the emitter is a rule in _DIFF_RULE_DASHBOARD ("do not use it on routine progress notes"), so the original bug recurs whenever the model forgets, and over-emission quietly re-defeats the user's collapse-all preference with no counterweight or telemetry.

Suggestions

  • Fold strip_control_comments into redact_for_display (or the one place the legs share) — every touched leg already comments "Strip-then-redact matches display_safe", so hoisting it removes the whole missed-leg class instead of enumerating sites.
  • Longer-term follow-up (beyond this PR): lift control tags into message meta at persist time — one strip site, and TurnBlock reads meta like isCrewReply already does.

[DESIGN-REVIEWED] 8e630e7

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

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

  • Veto branch shows users the raw tag. When a marked report contains an ambiguous fence (corpus cases "list-contained CLOSED fence: still vetoed", "over-indented fence line inside list continuation" — e.g. a fence indented 4+ under a wide list marker, or a blockquoted fence), strip_control_comments/stripKeepVisibleMarker do nothing, so Slack/Telegram mirrors, sidebar previews, Copy, and TTS ("keep-visible" spoken aloud via voice_reply.strip_markdown) all carry the literal <!-- keep-visible --> — and the report still folds, re-creating the original Collapse-all buries substantive mid-turn reports: no keep-visible bypass for deliverables between tool calls #7948 bug for exactly the code-heavy reports the prompt tells agents to mark. The PR's safety claim ("wrongly not stripping leaves an HTML comment the renderer never shows") holds only for the dashboard; channel formatters render it as visible text. Low-moderate frequency × minor friction × every such message. Smallest fix: at minimum document the residual on channel egress; better, treat a tag line that is the message's final line and outside any tracked fence as strippable even under veto on plain-text projections, where the false-strip cost is one lookalike line, not rendered content.

[UX-REVIEWED] 8e630e7

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 8e630e7058d28325ae93dccbb165a27556e730fa — 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 evidence gathered: producers verified (config/prompt.md:128 deliver instruction, task_planner.py:430 anchor appender), 8 backend call sites for the shared strip, frontend plan_task_id consumer at ChatPage.tsx:4333. Emitting the review.

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 ships

Intent: stop collapse-all from burying a substantive mid-turn report the user needed (#7948) — a FIX.

  1. Marked mid-turn report survives collapse-all (isKeepVisible in TurnBlock) — justified, the reported defect
  2. Dashboard prompt rule teaching agents to emit the marker — justified, the emitter half; correctly absent from channel variant
  3. Copy button omits the marker from pastes — rides along, justified (literal tag in paste)
  4. Search no longer phantom-matches inside the marker — rides along, justified, but see Watch
  5. Previews/TTS stop leaking deliver:/plan_task_id: tails too — declared rider, same root cause; producers verified (prompt.md:128, task_planner.py:430)
  6. Channel deliveries strip control tags (2 neutral sinks + 4 redact_via_context legs) — justified backstop; the legs bypass display_safe, so not double coverage
  7. TTS re-runs credential redaction post-strip — rider fixing a pre-existing rejoin class at cause level (covers all strips); protected by the AGENTS.md credential-redaction keep-list
  8. Shared tail-anchored, fence-vetoing grammar on both sides + conformance corpus — the mechanism; every bound and veto traces to a named content-loss failure mode

Watch

  • Item 4 is a point patch on the frontend: KEEP_VISIBLE_MARKER_RE strips only marker-led blocks, so a lone <!-- plan_task_id:… --> (emitted on every planner message, task_planner.py:430) or deliver: tail still phantom-matches in search and rides into copies. Counted: 2 unfixed sibling families, both already in the regex's stacked-tail alternation; the corpus documents the asymmetry as deliberate ("backend-only concerns"), but the phantom-match harm item 4 names applies to them identically. The one frontend consumer of the anchor (ChatPage.tsx:4333) reads m.content, not searchableText, so widening the strip breaks nothing.

Subtractions

  • Delete the two-scope asymmetry: let the frontend strip accept any trailing recognized-tag block (the backend's exact alternation, already present in the regex tail), keeping only hasKeepVisibleMarker marker-specific — removes the corpus's two "contract asymmetry" special cases and the divergence axis the corpus exists to police.

[FIRST-PRINCIPLES-REVIEWED] 8e630e7

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 8e630e7

Verdict parsed from the review's SHA-scoped output markers for commit 8e630e7058d28325ae93dccbb165a27556e730fa.

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

@patrigao
patrigao force-pushed the fix/7948-keep-visible-marker branch from 71e0221 to 9612a6d Compare September 2, 2026 19:16
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 8e630e7

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

@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: fixed — cross-surface gap (rule taught in _CRITICAL_RULES_TAIL, so channel sessions learn a marker no channel formatter strips).

Fixed in 9612a6d47: the rule moved out of the shared tail into the dashboard-only _DIFF_RULE_DASHBOARD block — exactly the per-runtime seam you named. _CRITICAL_RULES_CHANNEL no longer carries the marker, and TestKeepVisibleMarkerRule now pins the inverse contract (present in _CRITICAL_RULES, ASSERTED ABSENT from _CRITICAL_RULES_CHANNEL), so the wrong scope can't silently return. Channel formatters stay untouched: no channel agent is taught the marker, so there is nothing for them to strip.

@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: rebutted — suggestion to replace the marker-specific strip in searchableText.ts with the whole-class HTML-comment strip preview_text.py adopted.

The two sites are not symmetric: preview_text.py placeholder-protects code fences (_FENCE_RE runs FIRST), so its whole-class comment strip can never touch fenced content. searchableText.ts has no fence protection — its own header comment records the design rule that highlightable content (fenced code, prose) is deliberately left in so search never misses text the user can see highlighted. An HTML comment inside a fenced code block renders literally and IS highlightable, so a naive whole-class strip there would create the opposite defect: search misses on visible text. Fence-aware comment stripping for the frontend is a real improvement but a separate change beyond this PR's purpose — happy to file it as a follow-up if you want it tracked.

@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: fixed — Slack/Discord users would see literal <!-- keep-visible --> because the rule shipped in the shared tail while channel formatters never strip HTML comments.

Fixed in 9612a6d47 via your named smallest fix: the rule moved into the dashboard-only _DIFF_RULE_DASHBOARD block, so only dashboard sessions learn the marker, and the rule text now says "renders as nothing in the dashboard" rather than the unqualified claim. TestKeepVisibleMarkerRule asserts the marker is ABSENT from _CRITICAL_RULES_CHANNEL, pinning the scope you flagged the old test as cementing wrongly.

@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: fixed — subtraction 1 (shrink the rule's scope to the dashboard-only slot; drop the _CRITICAL_RULES_CHANNEL assertion).

Applied verbatim in 9612a6d47: the rule now lives beside _DIFF_RULE_DASHBOARD (your named seam), the shared tail carries nothing, and TestKeepVisibleMarkerRule was flipped from pinning the marker into BOTH variants to pinning it into the dashboard variant AND asserting its absence from the channel variant — the test now cements the correct scope instead of the wrong one.

@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: rebutted — subtraction 2 (retire the marker-specific spelling in searchableText.ts for the whole-class HTML-comment strip preview_text.py adopted).

The generalization is only safe where fences are protected. preview_text.py substitutes fence placeholders BEFORE its comment strip, so fenced content is untouchable there; searchableText.ts has no fence pass, and its documented contract is conservative stripping — highlightable content (fenced code, prose) stays in so search never misses text the user can actually see highlighted. An HTML comment inside a fenced block renders literally in the chat body, so the whole-class strip would trade the phantom-MATCH class for phantom-MISSES on visible text. You're right that <!-- deliver:... --> can still phantom-match dashboard search — that class predates this PR and its correct fix is a fence-aware strip, which I'd rather land as a follow-up than fold into this diff. Happy to file it if you want it tracked.

@patrigao
patrigao force-pushed the fix/7948-keep-visible-marker branch from 9612a6d to 97f0505 Compare September 2, 2026 20:45
@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: fixed — the strip-surface sweep was incomplete: voice_reply.strip_markdown strips [OPTIONS:] but not HTML comments, so TTS would speak the marker.

voice_reply.strip_markdown (src/kiro_crew/voice_reply.py:225) also strips [OPTIONS:...] … and its tag-removal regex (</?[a-zA-Z][^>]*>) does not match <!--. A dashboard session with voice reply on will TTS-speak the marker fragment of a marked deliverable.

Fixed in 97f050503, taking the suggested whole-class form: strip_markdown now strips all HTML comments (<!--[\s\S]*?(?:-->|\Z)), placed after the fence pass so code-block placeholders already protect fenced content — the same order-of-operations safety preview_text.py relies on. This also covers the dashboard Speak button, which routes handleSpeak → api.voiceSynthesize → strip_markdown. Tests added in test/test_voice_reply.py::TestStripMarkdown: marker + <!-- deliver:... --> removal, fence protection, unterminated-comment swallow (suite 166 passed).

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.

@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: fixed — Copy pasted a literal <!-- keep-visible --> tail on exactly the messages this feature targets.

The copy button ships steerCleaned (AssistantMessage.tsx:321), which strips [OPTIONS:] via parseOptions and steering tags — but no HTML comments. … Fix: apply KEEP_VISIBLE_MARKER_RE (or a general HTML-comment strip, matching the new preview_text.py behavior) on the copy path.

Fixed in 97f050503: the clipboard call site now ships steerCleaned.replace(KEEP_VISIBLE_MARKER_RE, '').trimEnd(). Marker-specific (not whole-comment) deliberately: copy is a fidelity-preserving action with no fence-protection pass, so a general comment strip would mutate fenced code the user is copying — the same reasoning that keeps searchableText.ts marker-specific. The raw-mode view stays verbatim (raw is the honest view). Test added: AssistantMessage.test.tsx renders a marked deliverable, clicks Copy, asserts the clipboard payload carries no control tag (119 passed).

Your second bullet (speak path): confirmed and covered by the same commit — onSpeak(content) routes handleSpeak → api.voiceSynthesize → voice_reply.strip_markdown, which now strips all HTML comments (fence-placeholder-protected), so TTS cannot read "keep-visible" aloud. Backend tests cover marker, deliver: tags, fences, and unterminated comments.

@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: fixed — the "two strip sites" claim was wrong; voice_reply.py was the counted unfixed sibling.

the description says searchableText and preview_text "were the two places the [OPTIONS:] marker is stripped," but grepping OPTIONS finds a third — voice_reply.py:225 strips [OPTIONS:...] for speech, and its tag-removal regex (line 209) does not match <!--, so a dashboard voice reply will speak "keep-visible". Same root cause; one-line fix in the same file family.

Fixed in 97f050503 with the one-line strip you predicted (<!--[\s\S]*?(?:-->|\Z) after the fence pass, mirroring preview_text), plus tests. The sweep was then re-run as a class enumeration rather than another point fix: plain-text projections of assistant content = searchableText.ts (marker strip, shipped round 1), preview_text.py (whole-comment strip, round 1), voice_reply.strip_markdown (whole-comment strip, this commit — also covers the dashboard Speak button via api.voiceSynthesize), and the Copy path in AssistantMessage.tsx (marker strip at the clipboard call site, this commit — copy has no fence pass, so whole-comment would mutate copied code). Channel formatters are excluded by design: channel agents never learn the marker (dashboard-only rule, round 1). The PR body's wrong claim is replaced with this four-site enumeration.

@patrigao
patrigao force-pushed the fix/7948-keep-visible-marker branch from 97f0505 to b383d8b Compare September 2, 2026 21:51
@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

span=69c47ee97daa
Disposition: fixed — a literal <!-- keep-visible --> quoted inside code matched the marker regex, so copy/search stripped visible content.

Literal marker in code -> copy/search replacement -> copied code loses content and search misses visible text.
Fix: Match only a standalone marker on the message's final line.

Fixed in b383d8b36 exactly as suggested: KEEP_VISIBLE_MARKER_RE is now tail-anchored — /(?:^|\n)[ \t]*<!--\s*keep-visible\s*-->\s*$/gi — matching only a standalone marker line at the end of the message, which is the emission contract (the prompt rule now reads "as its final line"). A marker quoted in a fenced block or mid-message prose no longer triggers the exemption, is not stripped from search text, and survives copy. The leading newline is consumed so copy leaves no trailing blank. Tests added: searchableText.test.ts asserts the tail marker is stripped AND a code-quoted marker is preserved; existing tail-marker fixtures (TurnBlock, copy) still pass (frontend 130 passed, tsc clean).

@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

span=f616f9d8abec
Disposition: fixed — the generic HTML-comment strips in preview_text.py and voice_reply.py deleted visible inline code (fence placeholders only protect triple-backtick blocks).

Assistant emits Use `<!-- ordinary -->` here -> preview or speech projection -> visible code becomes empty.
Fix: Restrict removal to recognized keep-visible and deliver control tags.

Fixed in b383d8b36 exactly as suggested: both sites now strip only recognized control tags — <!--\s*(?:keep-visible|deliver:[^>]*?)\s*--> — never all comments. An ordinary <!-- note --> (inline code or prose) survives previews and speech. The old unterminated-comment swallow is gone with the generic pattern (it was the same silent-data-loss shape: an unclosed <!-- ate the rest of the message); a truncated control tag now leaks literally rather than deleting text. Tests updated/added on both files: ordinary comments preserved (incl. the inline-code case from the finding), control tags stripped, fence placeholders unchanged, no-swallow locked in (backend 168 passed).

@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: accepted-and-deferred — the producer-side-only channel guard is a real residual, tracked in #8005 (label deferred-finding, assignee patrigao, due 2026-09-16).

the dashboard/channel rule variant is selected per message (source == "dashboard"), not per session. An agent that adopted the marker on earlier dashboard turns can emit it on a later channel-delivered reply in the same session, and nothing on the channel path strips it.

Mechanism confirmed (context.py selects the variant per message), so this is accepted rather than rebutted. Deferred because the smallest correct fix is wider than this PR: it touches four channel packages (slack/discord/telegram/webex), and the Slack path is a streaming renderer whose [OPTIONS:] handling uses a bracket-hold — a chunk-split marker needs equivalent hold logic, not a post-hoc strip. Meanwhile the leak needs a mixed-source session plus producer non-compliance (the channel prompt variant never teaches the marker), and costs one visible comment line — not data loss.

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 preview_text/voice_reply" — that generic shape no longer exists; GPT's round-3 BLOCKING findings on this head forced both sites to scope to keep-visible/deliver: tags because a whole-comment strip deletes visible quoted comments. Channel messages quoting comments in inline code have the same hazard.

On the second Watch bullet (compliance-dependence in both directions): inherent to the accepted [OPTIONS:] intent-marker precedent, as noted — the "only on content the user must see" guidance plus collapse-all's existing per-message manual toggle remain the guardrails; agreed it's worth watching in real transcripts, and no code change is proposed for it here.

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.

@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 Sep 2, 2026
Comment thread src/kiro_crew/voice_reply.py Fixed
@patrigao
patrigao force-pushed the fix/7948-keep-visible-marker branch from b383d8b to 50e48ff Compare September 2, 2026 22:57
@patrigao

patrigao commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

span=5791404b92d7
Disposition: fixed — comment strip could reconstruct a credential the pre-strip redaction scan never saw.

Split credential -> pre-strip scan misses it -> substitution rejoins it -> TTS exposes it.
Fix: Re-run URL and credential redaction after this substitution.

Fixed in 50e48ffee, widened from the instance to the class: the rejoin hazard is not unique to the new comment strip — [*_~]+ emphasis removal and the generic tag strip in the same function can rejoin a split key id the same way, and both predate this PR. strip_markdown now re-runs redact_exfiltration_urls + redact_credentials on its own output as its final step, so ANY strip-induced reconstruction is caught before the text reaches Polly or Piper. This also closes the split_sentences path, which called strip_markdown with no pre-strip redaction at all. Pre-strip redaction at the existing call sites stays (defense in depth + telemetry warnings).

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 TestStripMarkdown: comment-split rejoin (AKIAIOSF<!-- keep-visible -->ODNN7EXAMPLE → redacted), emphasis-split rejoin (AKIAIOSF**ODNN7EXAMPLE** → redacted), and contiguous-credential idempotence on the bare strip_markdown path. Backend suite 172 passed.

The same commit fixes the co-located CodeQL py/polynomial-redos alert on this line: the control-tag regex now keeps exactly one quantifier before the literal terminator (<!--(?:\s*keep-visible\s*|\s*deliver:[^>]*)-->), removing the overlapping-quantifier ambiguity; an adversarial-input wall-time test locks linearity.

@patrigao
patrigao force-pushed the fix/7948-keep-visible-marker branch from fffe160 to fb3d608 Compare September 3, 2026 03:37
@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: fixed — the counted unfixed sibling: the mirror-link backfill leg.

grepping redact_for_display(..., redact_via_context) finds three: chat_runner.py:2899 (fixed), slack/gateway.py:2950 (fixed), and chat_mirror.py:384 (not fixed) — the mirror-link backfill sends dashboard-authored assistant rows, exactly the marker-taught text, to Telegram/Discord/Teams with the literal tag intact. Same one-line strip-then-redact fix applies.

Correct count and correct class — your grep was the enumeration mine missed. Fixed in fb3d608cd with exactly the named one-line shape: redact_for_display(strip_control_comments(backfill_content(row)), redact_via_context) — strip-then-redact, same order as the other legs, redact_via_context untouched. The PR body's egress enumeration now reads three direct-egress legs, and the sinks+legs count closes at 8/8 backend projections.

@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: fixed — dangling doc pointer to the deleted constant.

preview_text.py:30 points readers at constants.CONTROL_COMMENT_RE, a symbol that does not exist (1 grep hit, the comment itself); the real symbol is _TRAILING_CONTROL_LINES_RE.

Fixed in fb3d608cd: the comment now names constants._TRAILING_CONTROL_LINES_RE. Repo-wide grep for the old name returns zero hits.

@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: accepted-and-deferred — the unenforced strip invariant; the class-deleting refactor is tracked in #8059 (label deferred-finding, assignee patrigao, due 2026-09-23).

The strip invariant is enumerated, not enforced … The next plain-text projection or egress leg added without calling strip_control_comments shows end users literal <!-- keep-visible -->, and no gate or shared chokepoint catches the omission — the grammar is centralized, the call sites are not.

Same class as your round-8 finding, and this round proved the point again in real time: First Principles found a third bypass leg (chat_mirror.py backfill, fixed in fb3d608cd), exactly the "next leg added without the strip" failure this concern predicts. That strengthens rather than changes the disposition: #8059 lifts the trailing tag block into message META at the single ingestion point (your crew_reply precedent), after which the per-site strips are deleted and a new projection is correct by default — enforcement by construction, not enumeration. Deferred because it spans persistence, the frontend exemption, heartbeat routing, and a legacy-transcript dual-read window; not a security/data-loss finding (cosmetic literal tag), so deferral is permitted, and all currently-known projections strip as of fb3d608cd.

@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: fixed — recognizer-pair drift, including the bound divergence you caught, plus your suggested conformance corpus.

The grammar now lives as two hand-maintained regex + fence-walker pairs (constants.py / keepVisibleMarker.ts) that already differ in bounds ([ \t]{0,16} vs [ \t]*); drift means the UI exempts a message whose marker a backend projection leaks, or vice versa, and nothing pins parity.
Suggestions: Add one shared conformance corpus (same input/expected pairs asserted by both the Python and TS test suites) so recognizer drift goes red instead of shipping.

Fixed in fb3d608cd, both halves. (1) The bounds are reconciled: the frontend regex now carries the backend's exact quantifiers ([ \t]{0,16}, \s{0,16}, body {0,256}) — the 17-space input that previously split the recognizers now reads as content on both sides. (2) Your corpus, verbatim design: test/fixtures/control_tag_corpus.json (15 input/expected cases — fence guards both delimiters, case, stacking, bound edges on both sides of 16, the unterminated-comment non-swallow) is asserted by BOTH suites (TestSharedConformanceCorpus in Python, an it.each block in keepVisibleMarker.test.ts reading the same file), so a bound or grammar edited on one side goes red on the other locally. The corpus also documents the one deliberate asymmetry (deliver-only tails strip on backend egress but are not the frontend exemption's business) as contract rather than drift.

@patrigao
patrigao force-pushed the fix/7948-keep-visible-marker branch from fb3d608 to a7c21fb Compare September 3, 2026 04:10
@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

span=f634241daeea
Disposition: fixed — two CommonMark conformance gaps in the shared grammar: over-generous tag-line indentation and a fence walker that accepted info-string closers. (Span hit count: 2 — round 7 hit this span on variable-backtick code spans, adjudicated by tail-anchor unification; this round's two shapes are conformance errors in that unified grammar's remaining markdown-aware parts, the indent bound and the fence walker, not a new dialect.)

Four-space-indented marker, or marker after ``` explanation inside an open fence -> stripping projections -> visible code disappears.
Fix: Accept only CommonMark's ≤3-space indentation and require fence closers to contain only trailing whitespace in both recognizers.

Fixed in a7c21fbf7, taking the named fix verbatim on BOTH recognizers: (1) tag-line leading indent is now [ \t]{0,3} — a ≥4-space-indented line renders as an indented code block, so it is visible content and never matches; (2) the fence walkers (_in_open_fence / inOpenFence) now require a closing delimiter to carry only trailing whitespace (CommonMark 4.5) — a fence-lookalike with an info string inside an open fence is literal code and leaves the fence open, so a tail after it stays swallowed and untouched. Four new shared-corpus cases pin both rules on both sides (3-space stripped / 4-space content; info-string non-closer keeps the tail; clean closer after the lookalike restores the strip), and the stale 16-space corpus case was corrected to the ≤3 contract. Backend 187 focused green, frontend 154, tsc clean. The remaining conformance surface of the grammar is now: line grammar (bounded, tail-anchored), indent rule (CommonMark 4.4), fence open/close (CommonMark 4.5) — each pinned by corpus cases asserted by both suites.

@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

span=38644d4f5240
Disposition: fixed — Copy silently trimmed trailing whitespace off unmarked messages.

Unmarked response ending with a newline or significant spaces -> Copy -> clipboard silently loses them.
Fix: Apply trimEnd() only when a marker was stripped.

Fixed in a7c21fbf7, named fix verbatim: the Copy handler computes the strip once and applies trimEnd() only when the strip changed the text (stripped === steerCleaned ? stripped : stripped.trimEnd()), so an unmarked message reaches the clipboard byte-identical while a marked message still sheds the whitespace the marker line left behind.

@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: accepted-and-deferred — strip-by-enumeration; the tracking issue you ask for exists: #8059 (label deferred-finding, assignee patrigao, due 2026-09-23).

There is no egress chokepoint, so the next outbound leg (new channel, new mirror) silently ships literal <!-- keep-visible --> to end users, and every future control-tag family multiplies the cost. Detecting the marker at message finalization (meta flag + one strip into stored content, as crew_reply already gates on msg.meta) would collapse N projection strips to one — a follow-up worth a tracking issue before the tag family grows.

#8059 carries precisely this design — lift the trailing tag block into message meta at the single ingestion point on the crew_reply precedent, store content pre-stripped, delete the N per-site strips, with a legacy-transcript dual-read note. Filed round 8, re-affirmed round 9 when First Principles' chat_mirror find demonstrated the predicted failure live. Deferred (not fixed in-PR) because it is an ingestion/persistence refactor across the frontend exemption, heartbeat routing, and task-planner consumers; cosmetic-literal-tag class, so deferral is permitted, and every currently-known projection strips as of a7c21fbf7.

@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Disposition: rebutted — the mid-message-emission gap is the deliberate, cheaper side of an adjudicated trade-off, kept as-is.

The tail-anchored contract means an agent that emits the marker mid-message (LLM non-compliance with "as its final line") gets neither the exemption nor the channel strip — the literal tag reaches channel users. Documented trade-off, cosmetic harm, but worth watching in real transcripts since the emitter is a prompt rule, not code.

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.

@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

span=f634241daeea
Disposition: needs-a-decision — same-span stall: this span has now landed a blocking finding in 3 rounds (7, 11, 12), which is the prepare-pr stall trigger. Per protocol I am NOT pushing a fourth point-patch; this is a maintainer ruling on which structural direction to take.

Message ending - ```\n <!-- keep-visible --> -> scanners miss the open fence -> channel delivery, copy, search, and speech delete code Markdown renders visibly.
Fix: Make both changed fence scanners recognize CommonMark container-prefixed fences.

Span history (the sibling series):

  • Round 7: variable-length backtick code spans defeated the code-preservation grammar → restructured to a tail-anchored grammar (deleted the code-span parser).
  • Round 11: the fence-parity guard (added round 9 for the unterminated-fence input GPT found in round 8) accepted 4+-space-indented tag lines and info-string closers → tightened to CommonMark 4.4/4.5.
  • Round 12 (this finding): the same guard misses container-prefixed fences (- ```` inside a list). Blockquote-prefixed (> ```` ) and nested-container fences are the visible next siblings; CommonMark container-prefix rules (marker-width-dependent continuation indent) make a regex walker an open-ended reimplementation of a block parser, in two languages, kept in lock-step.

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 keep_visible in message META, persist content pre-stripped, delete all projection-time recognizers. Under #8059 this entire span (all three rounds) is unreachable by construction.

Options — which do you want?

  1. One more conformance patch (GPT's named fix): teach both walkers CommonMark container prefixes. Smallest diff; risks a 4th sibling round (blockquotes, nesting) — the pattern so far says the reviewer will find it.
  2. Pull chat: lift trailing control tags into message meta at ingestion (deferred from #7960) #8059's meta-lifting into THIS PR: parse-at-ingestion, meta flag, content stored pre-stripped, delete the recognizers. Deletes the class and most of the per-projection strip sites; materially widens this PR (persistence + frontend exemption + legacy-transcript dual-read).
  3. Human /ai-review override gpt a7c21fbf7... accepting container-fence tails as residual risk (the input requires an agent to emit the marker inside a list-contained open fence at message end — a doubly non-compliant emission), merge on the current shape, land chat: lift trailing control tags into message meta at ingestion (deferred from #7960) #8059 as the scheduled follow-up (due 2026-09-23).

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 KEEP_VISIBLE_MARKER_RE unexport — staged as a single amended commit ready to ride with whichever option you pick.)

@patrigao
patrigao force-pushed the fix/7948-keep-visible-marker branch from a7c21fb to 6ff7966 Compare September 3, 2026 18:51
@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

span=f634241daeea
Disposition: fixed (in 6ff7966cf) — maintainer ruling received on the needs-a-decision escalation.

  • Finding (3rd hit on this span): fences opened inside CommonMark containers (list items - ```, blockquotes > ```) are not recognized as delimiters by either fence walker, so a marker after a list-contained open fence was stripped from derived surfaces — deleting visible fence-interior content.

Span history: hit 1 (round 7) variable-length backtick spans; hit 2 (round 11) indent bounds + bare-closer rule; hit 3 (round 12) container-prefixed fences. Three same-span rounds fired the prepare-pr stall rule; the escalation offered exact container conformance, in-PR meta-lifting (#8059), or an override. The maintainer rejected the override — silent content deletion from copies/previews is unacceptable even on exotic input — and ruled for a structural invariant instead of a fourth point-fix.

The fix (both walkers, constants.py + keepVisibleMarker.ts): strip only when PROVABLY outside every fence. A new over-approximate candidate detector (_AMBIGUOUS_FENCE_LINE_RE / AMBIGUOUS_FENCE_LINE_RE) matches any fence run preceded only by whitespace and container-marker characters (list bullets, ordered-list digits/punctuation, blockquote markers) or by 4+ spaces. A candidate seen while the exact walker believes it is outside any fence VETOES the whole decision: strip nothing, no exemption. Inside a tracked fence the same line shape is literal code under every interpretation and does not veto.

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.

@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • One grammar family is speculative: the tail deliver: strip has zero counted producers

Disposition: rebutted (grammar kept; the description clause it flagged is fixed — reworded in the current body).

Counted producers of a standalone tail deliver: line: 0 (heartbeat.py:109 writes them as same-line suffixes in HEARTBEAT.md; the only real tail emitters are the prompt rule and task_planner.py:430).

The code-emitter count is correct — verified: heartbeat._DELIVER_RE (line 34) is applied only to HEARTBEAT.md entry lines in _extract_tasks (line 364), never to agent response text, and line 109 writes same-line suffixes. But the count misses the prompt-side producer: the shipped system prompt (src/kiro_crew/config/prompt.md:128) instructs every agent to "Route completion with <!-- deliver:dashboard --> / <!-- deliver:slack --> tags" in its heartbeat completion response. Completion text is exactly what gets delivered to dashboard notifications and Slack, and an LLM following that instruction verbatim emits the tag as a standalone line — most naturally at the tail. That is the same producer class as the keep-visible marker itself: prompt-instructed, not code-emitted. Dropping the deliver alternative would let a prompt-induced emission render as a literal tag to channel users — the round-6 leak class this PR closes — to save one regex alternation and two corpus rows.

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 task_planner.py:430 and the prompt.md instruction as the producers and states that echoed HEARTBEAT.md suffixes are mid-body content the strip leaves alone.

@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • In-band control tag forces every current and future egress surface to know a duplicated fence-parsing grammar; one ingestion-time strip would collapse that fan-out

Disposition: accepted-and-deferred — this is the standing fan-out concern, adjudicated in rounds 8–11 and tracked as #8059 (deferred-finding label, assignee @patrigao, Due: 2026-09-23). The suggestion in this round's body — parse once at message finalization, set meta.keep_visible on the crew_reply precedent, persist content pre-stripped, keep the history strips as legacy backstop — is #8059's design verbatim.

Detect the tail marker once where the assistant message is persisted, set meta.keep_visible … and strip it from stored content — the grammar then lives at one chokepoint and every projection is clean by construction.

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
@patrigao
patrigao force-pushed the fix/7948-keep-visible-marker branch from 6ff7966 to 8e630e7 Compare September 3, 2026 19:41
@patrigao

patrigao commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

span=1fd39a49230f

  • \n\n\t<!-- keep-visible --> renders as code, but [ \t]{0,3} deletes it from all projections

Disposition: fixed (in 8e630e705).

Fix: allow only literal spaces in both recognizers.

Applied exactly that, to all four leading-indent classes: _TRAILING_CONTROL_LINES_RE and _FENCE_DELIM_LINE_RE (backend), KEEP_VISIBLE_MARKER_RE (both alternatives) and FENCE_DELIM_LINE_RE (frontend) now accept {0,3} — spaces only. A leading tab advances to column 4, which is indented-code territory per CommonMark, so a tab-indented tag line is visible content and is no longer stripped or exemption-eligible. The fence walkers get the same treatment for the sibling failure the finding implies: a tab-led ``` can no longer act as an exact delimiter (where it could wrongly CLOSE an open fence and re-enable a strip inside it) — tab-led fence runs fall into the ambiguity class, whose failure direction is do-nothing. Two new shared-corpus cases pin both sides: tab-indented marker preserved as content; tab-led fence run after an open fence keeps the tail swallowed. Backend 189 / frontend 158 tests green.

Note for the record: tsc -b on this branch reports 5 pre-existing errors in website/src/components/SketchDialog.tsx — verified present on pristine origin/main (183fdc323, introduced by #8041) in a detached-worktree probe; not touched by this PR.

@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 readiness: checking Automated validation is still running labels Sep 3, 2026
@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. Same file regions and the same six consumer surfaces for a parallel marker family; the two should agree on one shared strip pipeline rather than land two independently. Files: src/kiro_crew/constants.py, src/kiro_crew/preview_text.py, src/kiro_crew/voice_reply.py, src/kiro_crew/messaging/renderer.py, website/src/pages/chat/TurnBlock.tsx, website/src/utils/searchableText.ts.
  • This PR is OVERLAPPING with PR #6831. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7960: CONTINUE_DEVELOPMENT. Independent goals (channel visibility for /note lines vs collapse exemption) and complementary mechanics: 7960's display_safe strip is what keeps 6831's new leg from posting a literal control tag. Files: src/kiro_crew/dashboard/chat_slack.py.
  • This PR is OVERLAPPING with PR #8136. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7960: CONTINUE_DEVELOPMENT. Code-near in the same sink module but materially different behavior, and already reconciled — 7960 still merges cleanly over it. Files: src/kiro_crew/messaging/renderer.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

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

@bolichen97 bolichen97 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.

Approving — the diff is the collapse-all exemption fix it claims, with no visual delta.

Verified against the diff:

  • isKeepVisible in TurnBlock.tsx joins isVisibleInline exactly like the existing isHandBack [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 to findConclusionIdx, 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_RE and keepVisibleMarker.ts carry identical bounds (indent ≤3, whitespace ≤16, body ≤256) and identical fence walkers, pinned by the shared test/fixtures/control_tag_corpus.json asserted from both suites, so a bound edited on one side goes red on the other.
  • Fail-safe direction is right: _in_open_fence VETOES 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_for plus 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_markdown re-runs redact_exfiltration_urls + redact_credentials after 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_DASHBOARD and test_context.py::TestKeepVisibleMarkerRule asserts 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).

@bolichen97
bolichen97 merged commit bf4e88a into main Sep 4, 2026
65 of 66 checks passed
@bolichen97
bolichen97 deleted the fix/7948-keep-visible-marker branch September 4, 2026 18:14
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 4, 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.

Collapse-all buries substantive mid-turn reports: no keep-visible bypass for deliverables between tool calls

3 participants