fix(sdk): strip inline reasoning from LLM-generated titles

[aniketwaghh]

HUMAN:

Reproduced #4530 locally against a Qwen3 model served with mlx-lm: the autogenerated title came back as a truncated <think> block rather than the title the model actually produced. Fix and evidence below.

---

AGENT:

Why

generate_title_with_llm() takes response.message.content[0].text verbatim. A reasoning model whose provider does not split chain-of-thought into reasoning_content returns it inline as <think>…</think>, so the reasoning becomes part of the title.

The truncation makes it worse than a cosmetic prefix. With the default max_length=50, the title is cut to the first 47 characters, which are all reasoning, so the actual title is discarded rather than merely preceded by noise.

Summary

• Strip inline <think>…</think> blocks from the model's text before trimming and truncating.
• Treat an unterminated <think> as reasoning through to the end, since the response was cut mid-thought.
• Return None when nothing survives, so the caller falls back to its truncated-message title instead of storing an empty one.

Issue Number

Fixes #4530

Evidence

Reproduced against a real reasoning model rather than a fixture: mlx-community/Qwen3-4B-4bit on Metal via mlx-lm. It produced 1971 characters, verbatim:

<think>
Okay, the user wants a conversation title that starts with a message asking for
help summarizing a CSV in Python. The title needs to be concise, under 50
characters, and start with an emoji.

First, the main keywords here are "summarize CSV in Python". The emoji should
relate to coding or data. Maybe a 🧠 for thinking, but maybe a 📊 for data.
...
Help" with the emoji. So the final title would be "📊 Python CSV Summary Help".
</think>

📊 Python CSV Summary Help

Worth stating because it cuts against the result: MLX's own server does not exhibit this bug. It parses the thinking block into a separate field and returns clean content, so it is a well-behaved provider. What reproduces the bug is delivering that same model output in content, which is what the providers named in the issue do. The reasoning text above is real; the provider shape is the issue's.

Against HEAD~1 of this branch, which is unmodified main:

real model output      : 1971 chars, <think> present: True
model's actual title   : '📊 Python CSV Summary Help'

title stored           : '<think>\nOkay, the user wants a conversation tit...'
length                 : 50   (max_length default 50)
leaks reasoning        : YES
matches model's title  : NO

Same input on this branch:

title stored           : '📊 Python CSV Summary Help'
length                 : 25   (max_length default 50)
leaks reasoning        : no
matches model's title  : yes

The model produced a perfectly good title. Before this change it was discarded, because truncation to max_length consumed the reasoning first and never reached it.

A second real-model observation that motivated handling unterminated blocks: asked for a title with a 220-token budget, the same model emitted <think> and hit the cap before closing it. A response cut mid-thought has no title to salvage, so stripping only balanced blocks would leave the opening tag and the reasoning behind it in the title.

How to Test

uv sync
uv run pytest tests/sdk/conversation/test_generate_title.py -q

To see the behaviour rather than the assertions, check out HEAD~1 of this branch (unmodified main), feed a response whose text is <think>…</think>✨ Some Title through generate_title_with_llm, and observe the stored title truncate to 50 characters of reasoning. On this branch the same input yields ✨ Some Title.

Three tests added to tests/sdk/conversation/test_generate_title.py:

• inline reasoning is stripped and the intended title survives
• an unterminated block yields None, so the caller falls back
• a response with no reasoning is unchanged

Reverting only the source change turns the first two red and leaves the third green, which is the intended split: the third is a no-regression check, not a bug guard.

uv run pytest tests/sdk/conversation/test_generate_title.py   ->  12 passed
uv run pytest tests/sdk/conversation/                          ->  791 passed
uv run pytest tests/ -k title                                  ->  48 passed
uv run pre-commit run --files <changed>                        ->  ruff format, ruff lint,
                                                                   pycodestyle, pyright,
                                                                   import rules, tool registration
                                                                   all Passed

Note on scope

#4564 also touches title_utils.py but is about custom title prompts; I checked its diff for reasoning handling and there is none, so these are orthogonal beyond a possible textual conflict.

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once this is addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[enyst]

Thank you, @aniketwaghh ! It seems precommit is failing in CI, could you please take a look?

[aniketwaghh]

Fixed in 96564ec — the rebase onto main dropped the two blank lines between test_title_uses_real_http_transport and the first test I added, and ruff-format wanted them back. I'd only run make lint (ruff) locally; uv run pre-commit run --all-files now passes all eight hooks here, and the title tests are 15 passed.

[enyst]

Hi, I'm OpenHands-DeepSeek-v4-Pro, reviewing this PR autonomously on behalf of the maintainer. 👋

Taste Rating

🟡 Acceptable — clean, well-tested, and correctly evidenced; one divergence from the issue's stated acceptance criteria worth reconciling.

This is a genuinely good, small fix. The diagnosis is precise, the evidence section is unusually strong (real-model reproduction against mlx-community/Qwen3-4B-4bit, plus a before/after comparison on HEAD~1), and CI is fully green. The regex markers (<think> / </think>) match the exact \u003cthink\u003e format named in #4530, so the fix targets the right provider shape.

A few observations:

[IMPROVEMENT OPPORTUNITIES]

• [title_utils.py, strip_reasoning_blocks] Special case vs. stated requirement: #4530 acceptance criterion #3 explicitly asks for a conservative first-block-only peel so that a genuine mid-text literal <think> in a title is preserved. This implementation calls _REASONING_BLOCK.sub("", text) then _UNCLOSED_REASONING.sub("", text) — i.e. it strips every <think>…</think> block, and on an unterminated <think> discards everything from the first marker to end of string, regardless of position. A title that legitimately contained the marker (however contrived) would be truncated. That may be more robust for the actual goal of "never leak reasoning," but it diverges from what the issue explicitly specified and is not guarded by a test. Either (a) restrict the peel to the leading block, or (b) add a "mid-text literal preserved" test and note that strip-all is intentional. As written, test_generate_title_keeps_text_without_reasoning only covers a response with no reasoning at all — not a mid-text marker.

• [title_utils.py, strip_reasoning_blocks] Simplification (optional): the two-regex approach is clear and readable — no change required. Keeping two named constants is fine. Not worth bikeshedding.

Comments: docstrings are appropriately scoped and explain why an unterminated block means "no title to salvage." No noise.

Tests: The three new tests exercise the real generate → (mocked) completion → response-processing path and assert on the returned title/None, consistent with the file's existing conventions. They are not "assert the mock was called" tests — they fail if the strip logic regresses (the author verified by reverting the source change). Good.

Breaking-change check: The new return None on empty-after-strip is behavior-equivalent for callers — generate_title_from_message already treated "" as falsy and fell back to generate_fallback_title. None adds clarity and a diagnostic warning. No breakage.

---

VERDICT: ✅ Worth merging — sound, well-evidenced fix. The only real ask is to resolve the strip-all-vs-first-block divergence from the issue's acceptance criteria (add the mid-text test or document the intent).

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW — isolated to a single helper, purely additive text post-processing, no public API shape change, no dependency changes, no security surface, full CI green. The divergence above is a correctness nuance on a rare edge, not a blocking risk.

KEY INSIGHT: The fix is correct on the happy path but silently changes semantics from "peel the leading reasoning block" to "strip reasoning markers everywhere," a slightly broader (and untested-for) guarantee than the issue requested — make that intent explicit.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, teach me to do better: add a .agents/skills/custom-codereview-guide.md file to your branch (with triggers: [/codereview] in its frontmatter) documenting the context I'm missing, then re-request review — I read guidelines from the PR branch. See the customization docs.

[enyst]

thank you!