fix: strip reasoning blocks from AI outputs#317
Conversation
Optic00
left a comment
There was a problem hiding this comment.
Review
Good PR - the two closed gaps are real (reports rendered activeReport.content raw, and composeShareBody uploaded the raw summary), and I checked the new regex against the tricky cases (case-insensitive backreference, the $(?![\s\S]) end-of-string anchor under /m, the unclosed streaming block, backtracking behaviour on large unmatched input) - all sound. Two remaining gaps of the same class the PR is fixing, plus two smaller notes:
1. copyNotes still copies the raw summary to the clipboard
MeetingDetail.tsx:483 - const summary = meeting.summary?.trim() goes into navigator.clipboard.writeText unfiltered. Same pattern this PR fixes in composeShareBody, one screen up. With meeting.summary = "<think>private chain</think>\nPublic summary", the rendered note and the share upload are clean, but "Copy notes" puts the reasoning block on the clipboard. One-line fix, ideally in the same PR.
2. List preview and search snippets render the raw summary
PreviousRow.tsx(previewText, ~line 169) showsmeeting.summary?.trim()as the row preview on Home.CommandPalette.tsx:184passes rawm.summaryintosnippet()(noteSearch.ts:33), which renders at line 206. This one is double: the search itself also matches text inside reasoning blocks, so a query hit inside<thinking>...</thinking>surfaces reasoning text as the result snippet.
A summary that starts with a reasoning block shows that text in the meeting list and palette even though the detail view is now clean. Wrapping both sources in stripReasoning fixes display; whether search should also match stripped text only is a judgement call (I'd say yes, same input for index and snippet).
3. Backend tag list now lags the renderer
simple_recorder.py:79 - _REASONING_TAG_HEADER_PATTERN normalizes headers after think|thought only. The renderer now also knows thinking|reasoning, so <thinking>...</thinking>### Action Items inline would skip the parser normalization. Not a blocker, but worth syncing the lists (or extracting one source of truth) so they don't drift further.
4. Correction on the PR description (doesn't change the value of the PR)
The regex on main has no /m flag - /<(think|thought)>[\s\S]*?(?:<\/\1>|$)/gi - so $ already matched end-of-string, and it strips closed multi-paragraph blocks and unclosed streaming blocks correctly (I ran both cases against the old code to be sure). What the new version actually improves: the two extra tag names, no more blank-line residue between stripped blocks, and preserved indentation instead of trimStart(). Worth adjusting the description so the changelog doesn't record a bug that wasn't there.
Nits
- The "handles nested tags" test covers
<think>outer<thinking>inner</thinking></think>(different tags) but not same-tag nesting:<think>a<think>b</think>c</think>leaksc</think>with old and new regex alike. Pre-existing limitation and probably fine for real model output - just don't let the test name suggest it's covered. - Dropping
trimStart()is a deliberate contract change (the tests pin it), looks right to me - flagging it only so it's a conscious decision.
Happy to take 1-3 as a follow-up if you'd rather keep this PR small.
Thanks for the thorough review and for checking the regex edge cases. I'm working on fixes for the remaining issues you called out:
I'll also update the PR description to accurately reflect what the regex change improves, and I'll clarify the nested-tag test name so it doesn't imply same-tag nesting is supported. |
Let me know if you spot something else. |
Align the copy paths with the rendered views (and with ruzin#317, which makes the report view render stripReasoning'd content): both the Standard summary and an open report's markdown are stripped before hitting the clipboard. Seeded T1 report now carries a leading <think> block and the spec asserts it never reaches the clipboard.
|
Re-checked the follow-up commits - all four points and the nits are addressed:
I also re-ran the new regex shape ( Coordination note: #318 (just opened) rewrites the same |
Align the copy paths with the rendered views (and with ruzin#317, which makes the report view render stripReasoning'd content): both the Standard summary and an open report's markdown are stripped before hitting the clipboard. Seeded T1 report now carries a leading <think> block and the spec asserts it never reaches the clipboard.
…andard note (#318) * fix(meetings): Copy notes copies the open template report, not always the Standard note copyNotes built the clipboard payload from the Standard structured fields unconditionally, so with a generated template report on screen the copy silently disagreed with the displayed note. Extract the builder into lib/notesCopy.ts (pure, unit-tested) and route it through the component's activeReport: report open -> title + meta + the report's markdown, otherwise the Standard sections as before. Coverage: vitest unit tests for the builder + a T1 e2e spec driving the real pill switch and Copy notes action against a seeded meeting carrying a report (new STENOAI_E2E_SEED_REPORT seam in the mock IPC, gated so transcript-export.t1 keeps its report-less meeting). * fix(meetings): review hardening for Copy notes - Disable Copy notes while a summary/report stream is on screen (all non-idle phases are transient in-flux states) so the clipboard can't get the old note while the body shows streamed text (Codex finding). - Make the mock IPC's set-active-report stateful for the seeded report so the pill switch survives the post-mutation refetch like the real sidecar does, keeping the T1 spec faithful rather than relying on the re-sync effect's unchanged-dep skip. * fix(meetings): strip reasoning blocks in the Copy notes payload Align the copy paths with the rendered views (and with #317, which makes the report view render stripReasoning'd content): both the Standard summary and an open report's markdown are stripped before hitting the clipboard. Seeded T1 report now carries a leading <think> block and the spec asserts it never reaches the clipboard.
This PR hardens the output sanitization layer to ensure that reasoning blocks (e.g.,
<think>...</think>) from models never leak into user-facing views or exports. It expands support for additional model tags, preserves original Markdown formatting more cleanly, and applies the sanitizer universally across all UI components and features.Changes:
stripReasoningnow supports<thinking>,<thought>, and<reasoning>tags. It preserves legitimate Markdown indentation and cleanly removes blank-line residue after a block is stripped.simple_recorder.py) to handle the expanded set of reasoning tags, keeping frontend and backend parsing logic in sync.