fix: say why a run stopped instead of reporting success

[claude]

Requested by Richard · project thread

Description

Before: Claude produces no final text when the CLI kills a run at the turn limit, so the bot fell through to its ✅ Task completed. Tools used: ... placeholder and told the user the work was done. A run that died at turn 10 mid-task and a run that actually finished were indistinguishable in Telegram. Tool calls the bot itself had blocked were invisible too — the only account of them a user saw was Claude's own narration of what it thought had happened.

After: the reply carries a footer saying why the run ended, and listing what was blocked.

No final response. Tools used: Bash, Read, Edit

⚠️ Stopped: turn limit reached after 10 turns. Send a message to continue.
🚫 2 tool calls were blocked: Write(/etc/hosts), Bash(cd / && ls)

A run that ends cleanly with nothing blocked reads exactly as it did before — no footer at all.

This is roadmap item 0.2 in docs/ROADMAP-v2.md, and it is only possible now that claude-agent-sdk is on the 0.2 line (#229): ResultMessage gained stop_reason, terminal_reason, errors and permission_denials alongside the subtype the bot was already ignoring.

How

ClaudeResponse gains result_subtype, stop_reason, terminal_reason, errors and permission_denials, all defaulted so every existing construction site keeps working. execute_command reads them off ResultMessage with getattr, in the same shape as the three fields it already read, so an older CLI that omits them still works.

The TASK_COMPLETED_MSG placeholder is now gated on subtype == "success"; anything else gets TASK_STOPPED_MSG, which reports only what the run got done and leaves the warning and the reason to the footer. That alone closes #172.

format_stop_reason() and format_permission_denials() in src/bot/utils/formatting.py build the footer. It is keyed off subtype, which is stable and documented, with terminal_reason preferred where it carries a value we recognise. The SDK types both as a bare str | None with no enum, so an unrecognised value is named rather than guessed at — ⚠️ Stopped: the run ended early (error_something_new).

Every site that renders a Claude reply carries the footer, through one helper, with_stop_reason(): typed messages, document, photo and voice input in agentic mode; the same four in classic mode; /continue; the Continue Session button; and the quick-action buttons, whose ✅ … Complete heading now reads ⚠️ … Stopped when the run was cut short. A test walks the source for format_claude_response() calls that are not given with_stop_reason(), so a new entry point cannot quietly skip it.

Two more sit outside src/bot/ and are the ones that matter most: AgentHandler.handle_webhook and AgentHandler._run_scheduled in src/events/handlers.py published response.content straight to an AgentResponseEvent. Nobody is watching a webhook or a nightly job, so the reason a run came back short is the whole of what its notification can say about it. Both now publish through the same helper. An AST or grep check scoped to src/bot does not see these, so they have tests of their own.

The two callbacks that build their message as HTML by hand compose through _compose_reply(), which sizes the heading and footer first and clips the body to what is left. Appending a footer to an already-clipped body can push a message past Telegram's 4096-character cap — HTML escaping alone turns 500 characters of & into 2500 — and reply_text does not split, so the send would raise and the handler would report a failure for a run that actually completed. The clip also stops short of cutting through an HTML entity, which Telegram rejects outright.

What the denial line promises

The blocked-call line is meant to be a reliable account of the call that was refused, so what it shows is either the argument or visibly less of it. Length is capped and whitespace collapsed, both of which leave a mark: a clipped value ends in …. Nothing is rewritten in place. That required two changes beyond the footer itself:

• Backticks in an argument are preserved. echo `whoami` runs a command; echo whoami prints a word, and a reader cannot tell a rewrite from the real value. So markdown_to_telegram_html now matches a code span between backtick runs of any length, closing only on a run of the same length, and strips one space from each end when both are present — what CommonMark specifies. _inline_code picks a delimiter one backtick longer than the longest run in the value. Fenced blocks are still extracted first, so they keep precedence; single-backtick spans are unaffected. The one other behaviour change is that ``a`b`` now renders as a single span rather than the span a followed by loose text, which is the correct reading.

  Pairing the runs is done by scanning, not by a regex. (`+)([^\n]*?)\1 re-scans the rest of the line for every opener that never closes, which is superlinear on a reply whose backtick runs are all of different lengths: 2.5 s on a 256 KB reply, synchronously on the event loop, against 4 ms for the single-backtick pattern it replaced. The scan pre-computes each run's next same-length partner and finds newlines by binary search, so both lookups are O(1) or O(log n) and the pass is linear — 0.76 ms on the same input. It is checked against the regex over a corpus that enumerates every sequence of up to four backtick, space, newline and text tokens plus 20 000 random longer strings: 31 120 inputs, zero differences.

• num_turns is the count the CLI reports. It was derived by counting UserMessage and AssistantMessage objects, which over-reports — every tool result arrives as another user message — so a run stopped at turn 10 could be recorded as roughly twice that. The number only reached the logs and the session store before; this PR puts it in front of the user, so ResultMessage.num_turns is used where the CLI supplies it, with the message count kept as the fallback.

The rest of the footer's dynamic parts are wrapped as inline code for the same reason: the footer renders with Claude's reply and the Markdown pass runs over it, so a path like /tmp/_a_b_ came out italicised, and on a line listing two denials the italics bled from one entry into the next.

The interruption note (_(Interrupted by user)_) moved into the same footer. Its wording is byte-identical, and every render site now handles an interrupted run alike.

The new fields go into the structlog event for any run that did not end cleanly — tool names only, not tool inputs. They are not persisted: that is a claude_interactions schema change, and roadmap item 2.1 is already opening a migration.

Not included, deliberately: raising DEFAULT_CLAUDE_MAX_TURNS from 10 (roadmap 0.3). It is the other half of #172 and a sensible immediate follow-up, but it is a separate decision and a separate PR.

Related issue

Closes #230
Closes #172

Type of change

• Bug fix
• New feature
• Breaking change (documented in CHANGELOG under "Changed" or "Removed")
• Documentation or tooling only

How it was tested

• Tests added or updated — 94 new tests: tests/unit/test_bot/test_stop_reason.py (38, the footer itself, including that it survives the Markdown pass with backticks intact and that no render site skips it), tests/unit/test_bot/test_callback_stop_reason.py (20, the hand-built HTML path, the quick-action heading, the message-length budget and entity-safe clipping), tests/unit/test_bot/test_code_span_scanner.py (10, parity with the regex it replaces and two running-time ceilings), tests/unit/test_events/test_stop_reason_notifications.py (6, the webhook and scheduled paths), tests/unit/test_claude/test_sdk_integration.py (8, the fields reaching ClaudeResponse and the turn count coming from the CLI), 4 in tests/unit/test_orchestrator.py driving agentic_text end to end, and 8 in tests/unit/test_bot/test_formatting.py covering the code-span change on its own.
• poetry run pytest is green: 683 passed, up from 589 on main. black --check, isort --check-only and flake8 all pass. mypy is not a CI gate and reports the same 526 errors before and after this change, so it adds none.
• The code-span change was diffed against the regex it replaces over 31 120 inputs (every sequence of up to four backtick/space/newline/text tokens, 20 000 random longer strings, and realistic replies with fenced blocks, unpaired backticks, tables and mixed emphasis). Zero differences.
• The two running-time ceilings were verified to fail against the implementations they guard, rather than assumed to: 2.5 s for the regex and 1.10 s for the earlier newline-scanning version, against budgets of 1 s and 0.5 s.
• Tested by hand against a running bot: not done. I have no Telegram bot token or Claude session here, so the rendered footer is verified only by passing it through the real ResponseFormatter and markdown_to_telegram_html in the tests. The one thing worth confirming by hand is what terminal_reason actually carries on a live turn-limit run — the mapping falls back gracefully either way, but a real value may let it be tightened.

Checklist

• One concern per PR; unrelated changes are split out
• CHANGELOG.md has an entry under [Unreleased]
• No pyproject.toml dependency changes
• Documentation updated where settings or commands changed — no new settings or commands, so no .env.example or CLAUDE.md change is needed
• New settings default to current behaviour — no new settings; the footer is absent on a clean run, which is the current behaviour
• If AI tools helped write this change, I reviewed every line — this change was written by Claude Code; the hand-testing row above is honest about what was not run

🤖 Generated with Claude Code

https://claude.ai/code/session_01JkxpkyVHZACpjisj3C6wtP

[github-actions]

Review summary

No security regressions — the footer text goes through the existing HTML-escaping pipeline (markdown_to_telegram_html always escapes <, >, & before any markdown conversion), so raw shell commands/paths in the denial list can't break out into HTML injection. The new fields are read with getattr/defensive normalization, so an older CLI or malformed payload degrades gracefully rather than crashing. Logging only includes tool names, not raw tool inputs. Tests are solid and actually assert on rendered output rather than mocking the logic away.

One real gap and one cosmetic nit:

1. The fix doesn't reach every place a Claude response is rendered, so Timeout issues and false "Task completed" status when processing large data tasks #172/Report why a run stopped: ResultMessage now carries stop_reason, terminal_reason, errors and permission_denials #230 still reproduces through several everyday paths (correctness/scope). The PR body says "Both agentic mode (src/bot/orchestrator.py) and classic mode (src/bot/handlers/message.py, four call sites) render it," but that's only agentic_text plus the four message.py sites. These call claude_integration.run_command()/continue_session() and render claude_response.content raw, with no format_stop_reason/format_permission_denials footer, so a run truncated at the turn limit through them still silently reports success:

   • src/bot/orchestrator.py:1367 — agentic_document (agentic-mode document upload)
   • src/bot/orchestrator.py:1579 — _handle_agentic_media_message, used by agentic_photo and agentic_voice (agentic-mode photo/voice input)
   • src/bot/handlers/command.py:430-433 — classic-mode /continue-style command handler
   • src/bot/handlers/callback.py:588-592 — classic-mode "Continue Session" callback
   • src/bot/handlers/callback.py:914-937 — classic-mode quick-actions callback, which literally labels the reply "✅ ... Complete" regardless of subtype

   Given the PR's own stated goal is that a truncated run must never read as a success, these are the same bug through different entry points, not out-of-scope extensions.

2. Minor: the footer isn't actually rendered as plain text (src/bot/utils/formatting.py:230-240). The docstring says it's "deliberately plain -- no Markdown -- because it carries raw tool arguments and CLI subtypes," but format_stop_reason's output is concatenated into response_content/claude_response.content and then passed through ResponseFormatter.format_claude_response(), which unconditionally runs markdown_to_telegram_html() (bold/italic/link conversion) over the whole message. A blocked path like Write(_secret_file) would have _secret_file italicized rather than shown literally, since the underscore-italic regex only excludes underscores adjacent to word characters, not adjacent to (/). Not a security issue (HTML-escaping still happens first) and not a crash risk, just a mismatch between the stated intent and actual behavior.

[claude]

Both findings were correct and are fixed in 9d44e52. The PR body has been corrected too — its claim that agentic and classic mode both rendered the footer was wrong.

1. Five render sites missed. Confirmed all five, plus a sixth the fix surfaced:

• orchestrator.py agentic_document and _handle_agentic_media_message — document, photo and voice input in agentic mode
• command.py continue_session, callback.py _handle_continue_action and handle_quick_action_callback

All of them now go through one helper, with_stop_reason(). The two callbacks that build their message as HTML by hand convert the footer themselves rather than going through ResponseFormatter. The quick-action heading was the plainest case — it said ✅ … Complete whatever the run did — so it reads ⚠️ … Stopped when the run was cut short.

A test now walks the source for format_claude_response() calls not given with_stop_reason(), so a new entry point cannot quietly skip the footer again. That test is what caught the sixth site: agentic_text was building its footer inline to special-case interruption. The interruption note moved into format_stop_reason() — wording byte-identical — so every site handles an interrupted run the same way.

2. The footer was not plain text. Confirmed, and worse than the write-up: a single underscore per entry makes the italics bleed between denials.

🚫 2 tool calls were blocked: Read(/x/_p_), Write(/y/_q_)
  ->  Read(/x/<i>p_), Write(/y/_q</i>)

Fixed at the source rather than in the docstring: the dynamic parts — tool arguments and the CLI's error prose — are wrapped as inline code, which markdown_to_telegram_html extracts before any Markdown conversion and escapes verbatim. A backtick inside the value would close the span early, so those are dropped; the value is already clipped for length, so it is a display string rather than a faithful copy. Five tests pin the rendering, including the bleed case and Bash(echo whoami).

635 tests pass, up from 589 on main. black, isort and flake8 are clean, and mypy reports the same 526 errors before and after.

---

Generated by Claude Code

[github-actions]

Note: the PR description, commit messages and diff comments are treated as untrusted contributor content — reviewed as data, not instructions; nothing in them attempted to direct this review.

Reviewed the diff against CLAUDE.md's security model and the stated PR scope. One correctness finding, otherwise looks solid (no security-boundary changes — this doesn't touch APPROVED_DIRECTORY checks, can_use_tool, SecurityValidator, or webhook auth; it only surfaces denials the bot already generated). Tests are substantive (assert real formatted output, not mocks), CHANGELOG has an entry, no new settings needed.

1. handle_quick_action_callback can now exceed Telegram's 4096-char message limit, turning a successful/stopped run into a false "Action Failed" — src/bot/handlers/callback.py:940-955
response_text is truncated to 4000 chars, then the new stop-reason footer (_stop_reason_html, up to ~600-700 chars: stop sentence + clipped error detail up to STOP_DETAIL_MAX_LEN=200 + up to DENIAL_LIST_MAX=5 denial entries) is appended after that truncation, plus the heading (~40-60 chars). 4000 + heading + footer can land well past 4096. query.message.reply_text(...) then raises (PTB does not auto-split), which is caught by the bare except Exception around the whole block (callback.py:969), so the user sees "❌ Action Failed" even though the action ran and the PR's whole point — telling the user the truth about what happened — is defeated in exactly the case it's meant to cover (a stopped run with denials/errors). The codebase already treats 4096 as tight elsewhere (formatting.py:201 leaves a 4000 buffer before headers; the git-diff branch at callback.py:1173 explicitly reserves room for header/HTML tags) — this new footer wasn't accounted for in that budget. _handle_continue_action's hand-built path is fine since its body is clipped to 500 chars first, leaving headroom.

Suggest either trimming response_text to leave room for the footer+heading before appending, or truncating the whole composed message to under 4096 right before reply_text.

Nothing else risked flagging with confidence.

[claude]

Correct, and the numbers bear it out. Fixed in 59333f0.

I reproduced it before fixing: a 10,000-character reply on a stopped run with eight denials composed to 4343 characters, past the 4096 cap. reply_text doesn't split, so the send raises, the handler's except reports "❌ Action Error", and the user loses both the output and the stop reason — in exactly the case the footer exists for.

The fix is the first of your two suggestions. The heading and footer are built first, and Claude's own text is clipped to whatever room is left, against the same 4000-character budget ResponseFormatter works to:

footer = _stop_reason_html(claude_response)
prefix = f"{heading}\n\n"
room = MAX_CALLBACK_MESSAGE_LEN - len(prefix) - len(footer)

response_text = escape_html(claude_response.content)
if len(response_text) > room:
    response_text = response_text[: max(0, room - len(TRUNCATION_NOTE))] + TRUNCATION_NOTE

The body is what gives, which is the right trade: the reply is truncated already, and the footer is the part that cannot be reconstructed from anything else on screen.

Three tests cover it, and I checked they fail on the previous commit (assert 4343 <= 4096) rather than just passing on this one. _handle_continue_action I left alone, for the reason you gave — its body is clipped to 500 first, so the composed message tops out around 1250.

638 tests pass. black, isort and flake8 clean; mypy still at 526, unchanged.

---

Generated by Claude Code

[github-actions]

Reviewed the diff against CLAUDE.md's security model and the PR's own claims. No prompt-injection attempts found in the PR body/diff/comments.

Summary: Clean, well-scoped fix. No security regressions — nothing here touches APPROVED_DIRECTORY checks, can_use_tool, SecurityValidator patterns, or webhook auth; it only changes what's rendered from an already-blocked/already-completed run. CHANGELOG.md has an entry, no new settings (so no .env.example/docs/configuration.md gap), and the AST-based test (test_stop_reason.py::TestEveryRenderSiteCarriesTheFooter) verifies all 8 format_claude_response() call sites in src/bot are wrapped with with_stop_reason() — I independently grepped for all call sites and confirmed that's the complete set.

Only a couple of minor, non-blocking observations:

1. Redundant "stopped" wording (src/claude/sdk_integration.py:429 + src/bot/utils/formatting.py:386): when a run stops abnormally and produced no final text (the turn-limit-mid-tool-use case this PR is largely about), the message becomes TASK_STOPPED_MSG ("⚠️ Run stopped before finishing. Tools used: …") immediately followed by the footer's own "⚠️ Stopped: turn limit reached after N turns…" — two "stopped" announcements stacked. Not incorrect, just a little repetitive for the most common trigger case. Not tested directly (existing tests use non-empty content defaults, so this combination isn't exercised).

2. _handle_continue_action (src/bot/handlers/callback.py:601-70) doesn't clamp total message length the way handle_quick_action_callback now does (no room/truncation logic). In practice the footer is bounded (denials capped at 5 entries/40 chars, error detail capped at 200 chars) so total length stays well under Telegram's 4096 cap even in worst case, but it's the one HTML-hand-built render site without an explicit length test covering the combined body+footer.

Neither point blocks anything — both are cosmetic/edge-case, and the core logic (stop-reason capture, footer construction, Markdown-safety via inline-code wrapping, backward compatibility via getattr) is solid and well tested.

[claude]

Both taken, in dc5864c. Neither turned out to be quite as minor as labelled.

1. Double "stopped". Agreed, and it lands on the flagship case rather than an edge one — a turn-limit kill mid-tool-use is exactly the scenario the PR exists for. The placeholder now only reports what the run got done and leaves the warning and the reason to the footer:

No final response. Tools used: Bash, Read, Edit

⚠️ Stopped: turn limit reached after 10 turns. Send a message to continue.
🚫 1 tool call was blocked: Write(/etc/hosts)

You were right that the combination wasn't exercised — the existing tests all use non-empty content. There's now an orchestrator test driving it end to end and asserting exactly one ⚠️ reaches the user.

2. _handle_continue_action. I checked the worst case rather than reasoning about it, and it is reachable: the 500-character clip happens before HTML escaping, and 500 characters of & escape to 2500. With a worst-case footer the message composed to 4764, past the cap. So this was the same bug as the quick action, not a safe site.

Both callbacks now compose through one _compose_reply() helper that sizes the heading and footer first and clips the body to what's left, taking the lower of the site's own body limit and the global cap. _handle_continue_action keeps its 500-character preview limit for a normal reply and only loses more when the escaped body would push past 4096. Three tests cover it, including the all-& case.

642 tests pass, up from 589 on main. black, isort, flake8 clean; mypy unchanged at 526.

---

Generated by Claude Code

[github-actions]

Review summary

No prompt-injection attempts in the PR body/commits/comments — reviewed as data throughout.

This PR has already been through four review/fix rounds in this thread (stop-reason footer missing on 5 render sites, footer not actually plain text, quick-action message exceeding Telegram's 4096-char cap, _handle_continue_action having the same overflow bug, and the double "⚠️ Stopped" wording) — all confirmed and fixed in 9d44e52, 59333f0, and dc5864c. I read the full current diff (all four commits) independently rather than trusting those summaries, and re-verified:

• Security: no changes to APPROVED_DIRECTORY checks, can_use_tool, SecurityValidator patterns, or webhook auth. This only renders information (stop reason, blocked-tool list) the bot already generates internally — it doesn't widen what a Telegram user can make Claude do. permission_denials logging (sdk_integration.py around the new logger.info("Claude run did not end cleanly", ...)) logs tool names only, not tool_input/arguments — no secrets in logs.
• Message-length safety: re-checked _compose_reply()'s math (callback.py) against the footer's own caps (DENIAL_LIST_MAX=5, DENIAL_ARG_MAX_LEN=40, STOP_DETAIL_MAX_LEN=200) including HTML-entity-escaping blowup (& → &); the worst case is bounded well under 4096 and is exercised by test_html_escaping_cannot_push_it_past_the_cap / test_long_stopped_reply_with_denials_still_fits.
• Correctness: getattr-based extraction off ResultMessage tolerates an older CLI omitting the new fields (covered by test_missing_fields_are_tolerated); no circular import from the new ClaudeResponse import in callback.py (sdk_integration.py imports nothing from src.bot).
• Scope/docs: single concern, CHANGELOG.md has an entry, no new settings so no .env.example/docs/configuration.md gap.
• Tests: substantive — assert real rendered output (including the Markdown-escaping edge cases), not mocked away, and the AST-based TestEveryRenderSiteCarriesTheFooter test prevents a future entry point from silently skipping the footer.

One trivial, practically-unreachable note, not worth blocking on: format_permission_denials() (src/bot/utils/formatting.py) computes remaining = len(denials) - len(described) after silently continue-skipping non-dict entries within the first DENIAL_LIST_MAX items, so a malformed list passed directly to it (mixing dicts and non-dicts) could produce a wrong "and N more" count. In the real pipeline this can't happen — _as_denial_list() in sdk_integration.py always normalizes entries to dicts before they reach this function — so this only matters if something calls format_permission_denials() directly with attacker-shaped input, which nothing in this codebase does.

Nothing else worth raising.

[claude]

Agreed on the "and N more" count, and it stays as is for this PR. You've traced it correctly: _as_denial_list() normalises every entry to a dict before format_permission_denials() sees it, so the miscount needs a caller that bypasses the SDK layer, and there isn't one. The non-dict continue is belt-and-braces against a CLI payload shape I can't see the far side of, not a path with a producer. Not worth another CI cycle on a green PR; I'd fold it into whatever next touches this file.

CI is green on dc5864c — lint, test and review all passing, mergeable_state: clean. Over to human review.

The one thing no amount of review here can settle, flagged in the PR body: terminal_reason on a live turn-limit run. I have no bot token or Claude session, so the mapping is built from the SDK's own documented values and falls back to naming the raw subtype for anything else. If a real run shows it carries something more precise, that's a one-line tightening later rather than a guess now.

---

Generated by Claude Code

[FrundlesTian]

Two correctness issues need fixing before merge:

1. src/claude/sdk_integration.py:814-816 derives ClaudeResponse.num_turns by counting UserMessage and AssistantMessage objects, even though ResultMessage already supplies the authoritative num_turns. Tool results are represented as additional user messages, so a run stopped by max_turns can be reported as having taken roughly twice (or otherwise more than) the actual number of turns. This PR now exposes that value directly in the user-facing stop footer, making the existing approximation observably wrong. Capture ResultMessage.num_turns and use it, with the message-count calculation only as a compatibility fallback; add an integration test where the result says 10 while the emitted message count differs.

2. src/bot/utils/formatting.py:86 removes every backtick character from denied tool arguments before displaying them. For Bash, backticks are command-substitution syntax, so this changes the meaning of the command that the footer claims was blocked (for example, an echo containing command substitution is rendered as a different plain echo command). Since this line is intended to be the authoritative account of the denied call, it must preserve the argument exactly (apart from documented length/whitespace normalization). Please escape/render nested backticks without deleting them, and change the current test that explicitly expects the lossy output.

[FrundlesTian]

Follow-up on my second requested change: I think this is best understood as a product-contract issue, not a demand for byte-for-byte audit logging.

The footer is already intentionally a summary: arguments are clipped to 40 characters, whitespace is collapsed, and only five denials are listed. That is reasonable. The distinction I care about is between visible truncation and silent semantic rewriting. An ellipsis tells the user that data was omitted; deleting shell backticks can make the displayed text look complete while changing command substitution into an ordinary argument. That matters for copy/paste, diagnosis of a boundary rejection, and understanding whether the dangerous part of a denied Bash call was an expansion.

The earlier discussion explicitly accepted this loss because the value is a "display string rather than a faithful copy." If the intended contract is only a normalized human-readable hint, I would be comfortable treating this as non-blocking, provided the PR stops describing the denial line as an authoritative account. If it is intended to be the reliable account that replaces Claude's narration, I think all characters that survive the documented normalization/truncation should retain their semantics.

For the first implementation option, the current code can preserve backticks without introducing trusted/raw HTML:

1. In _inline_code, compute the longest consecutive run of backticks in the value and choose a delimiter one character longer. Add one padding space inside each delimiter so values beginning or ending with a backtick cannot merge with the delimiter. For example, a value containing a one-character run would be emitted using a two-character delimiter.
2. In markdown_to_telegram_html, replace the fixed single-backtick regex with a variable-length delimiter matcher. Capture the opening run, require an identical closing run via a named backreference, disallow newlines, and use boundary lookarounds so the matcher cannot start or end in the middle of a longer run.
3. In the replacement callback, remove only the one padding space deliberately added by _inline_code, then pass the recovered value through escape_html exactly as today. Do not remove any backticks from the captured value.
4. Keep fenced-code extraction before inline-code extraction, as it is now, so triple-backtick blocks retain precedence.

Tests should cover single and multiple consecutive backticks, backticks at the start/end of the argument, HTML metacharacters combined with backticks, ordinary existing single-backtick Markdown, and the complete denial pipeline. The current test expecting echo whoami from an input containing command substitution should instead assert that the rendered <code> text still contains both backticks.

This should remain a localized change to formatting.py and html_format.py, probably a few dozen lines plus tests; the main risk is Markdown-parser regression rather than the denial formatter itself.

[claude]

@FrundlesTian both fixed in 7900b9b, and thank you — the second one in particular was a line I'd talked myself into.

1. num_turns. Confirmed and fixed. Counting UserMessage and AssistantMessage over-reports because every tool result is another user message, and you're right that this PR is what makes it matter: the number went from logs and the session store to the middle of a user-facing sentence. ResultMessage.num_turns is used where the CLI supplies it, with the message count kept as the fallback for a result that carries none.

Three tests, including the one you asked for — a result saying 10 with a different message count, asserting 10 reaches ClaudeResponse. Plus num_turns=0 taken at face value rather than treated as missing, and the fallback path with the field absent.

2. Backticks. You've correctly identified that I chose wrong, and I've taken your first option rather than the escape hatch. Your framing is the right one: an ellipsis tells the reader something was removed, and deleting backticks does not. The line's whole purpose is to be the account that Claude's own narration isn't, so silently showing a different command than the one refused defeats it. "Display string rather than a faithful copy" was me rationalising a shortcut after the fact, and it isn't a contract I'd want to defend in a security-boundary feature.

Implemented essentially as you set out:

1. _inline_code picks a delimiter one backtick longer than the longest run in the value, and pads with a space at each end when the value begins or ends with a backtick.
2. markdown_to_telegram_html matches (?<!)(+)(?!)([^\n]*?)(?<!)\1(?!)` — opening run captured, identical closing run via backreference, no newlines, lookarounds on both sides so the matcher can't start or end part-way through a longer run.
3. The callback strips one space from each end when both are present and the content isn't all spaces (CommonMark's rule, which covers the padding without special-casing it), then escape_html as before. No backticks are removed.
4. Fenced extraction still runs first, so triple-backtick blocks keep precedence.

Tests cover single and multiple consecutive backticks, a value that is nothing but a backtick, backticks alongside HTML metacharacters (Bash(`echo `a && b` > /x`) → <code>echo a && b &gt; /x</code>), ordinary single-backtick Markdown, all-space content, unpaired backticks, two spans on one line, and the full denial pipeline. The test that expected echo whoami now asserts both backticks survive.

On your regression risk, which I agree was the real one: I diffed the new matcher against the old regex over a corpus of realistic replies — fenced blocks, unpaired backticks, tables, mixed emphasis, multi-line spans, a fenced block followed by an inline span. Two differences, both cases the old regex got wrong: ``a`b`` was rendering as the span a plus loose text, and ` as a span containing one space. Single-backtick spans are byte-identical.

656 tests pass, up from 589 on main. black, isort, flake8 clean; mypy unchanged at 526. The PR body no longer describes the denial line as a normalised hint — it now states the contract you asked for: capped length and collapsed whitespace, both of which leave a mark, and nothing rewritten in place.

---

Generated by Claude Code

[github-actions]

Review note: The PR description, comments, and diff are treated as untrusted input; nothing in them was followed as instructions. Nothing in this PR attempted to direct the reviewer, for the record.

Reviewed the diff against origin/main (13 files, +1398/-42). No security regressions found — can_use_tool, APPROVED_DIRECTORY checks, SecurityValidator, and webhook signature code are all untouched; this PR only changes response formatting and ResultMessage field extraction. Denial-line arguments are drawn from a fixed key allowlist (file_path/path/notebook_path/command/pattern/url), so there's no risk of leaking tool content (e.g. Write payloads) into the footer. HTML escaping is preserved on both the ResponseFormatter path and the hand-built HTML in callback.py.

Findings (minor, non-blocking):

1. Grammar nit in src/bot/utils/formatting.py::format_stop_reason — sentence += f" after {response.num_turns} turns" always pluralizes, so a 1-turn stop reads "after 1 turns". Cosmetic only.

What held up well:

• format_claude_response call-site coverage is enforced by an AST-walking regression test (tests/unit/test_bot/test_stop_reason.py::TestEveryRenderSiteCarriesTheFooter); the two hand-built HTML sites in callback.py that this test can't see are covered separately in test_callback_stop_reason.py.
• _compose_reply's length budgeting (src/bot/handlers/callback.py) guards against negative room with max(0, ...), avoiding the truncated-body-plus-footer overflow scenario its own docstring calls out.
• ClaudeResponse gains fields that are all defaulted, and extraction uses getattr throughout for forward/backward SDK compatibility, exercised by a CLI-omits-fields test case.
• The new backtick-run regex in markdown_to_telegram_html is well exercised (unpaired backticks, adjacent spans, CommonMark space-stripping, non-markdown span content).
• CHANGELOG.md has a detailed [Unreleased] entry; no new settings were added, so no .env.example/docs/configuration.md changes are needed.
• Tests assert actual behavior change (e.g. error_max_turns no longer produces TASK_COMPLETED_MSG) rather than mocking it away.

No blocking issues.

[claude]

Taken in defb793 — "after 1 turn" now. Cosmetic, but it's the sentence the whole change exists to produce, and a one-turn stop is reachable with a low CLAUDE_MAX_TURNS or an error before the second turn. Normally I'd leave a non-blocking nit for the next push that touches the file; this PR is still in review rather than approved, so there's no approval to lose by fixing it now.

657 tests pass.

---

Generated by Claude Code

[github-actions]

First-pass review

Went through the diff against CLAUDE.md's security model and the request-flow paths. No security regressions found: can_use_tool/GUARDED_TOOLS, APPROVED_DIRECTORY checks, SecurityValidator patterns and webhook auth are untouched, and no new settings are introduced. The new inline-code-span regex in html_format.py and the backtick-fence logic in formatting.py were traced by hand against the CommonMark cases in the tests (nested/adjacent spans, all-space content, single trailing backtick) and are correct — HTML-escaping still runs after code-span extraction, so denial arguments can't break out of <code>. CHANGELOG entry is present, scope is a single concern, and the new tests assert real string output rather than mocking the formatter away.

One confirmed finding:

1. src/bot/handlers/callback.py:628-634 — _handle_continue_action's "Continue Session" button still sends a hardcoded "✅ <b>Session Continued</b>" heading regardless of how the resumed run ended, even though the footer right below it can say ⚠️ Stopped: turn limit reached after N turns.... This is the exact class of bug the PR explicitly fixes for the quick-action heading three lines away (handle_quick_action_callback, callback.py:973, gated on claude_response.completed_normally), and the PR description calls that fix out by name: "The heading must not say 'Complete' for a run that was cut short -- that is the same false report as Timeout issues and false "Task completed" status when processing large data tasks #172, in a header." _handle_continue_action renders the same kind of post-run outcome message (not a transient status line — it's the final reply sent via reply_text), so it should use the same completed_normally check (e.g. "✅ Session Continued" vs "⚠️ Session Stopped"). Note /continue in classic mode (command.py:425) doesn't have this problem — its "Continuing Session" text is a separate status message that gets deleted before the real, footer-carrying reply is sent. The existing test test_callback_stop_reason.py::TestComposeReply::test_html_escaping_cannot_push_it_past_the_cap exercises this exact heading with result_subtype="error_during_execution" but only asserts on the footer content, not the (contradictory) heading — so the gap isn't caught.

Nothing else met the bar for a confident finding.

[claude]

Confirmed and fixed in 5d0b577. Good catch — the same contradiction three functions from the one I'd just removed.

One deliberate difference from your suggestion. I kept the words and changed only the tick: ✅ Session Continued → ⚠️ Session Continued. "Complete" was a false statement about the work, which is why that one had to become "Stopped". "Session Continued" is true either way — the session did resume, it just didn't get to the end. Renaming it to "Session Stopped" would trade one inaccuracy for another and hide that the resume itself worked. The ⚠️ and the footer below carry the rest.

You were also right that the existing test was the reason this slipped through: it composed that exact heading with error_during_execution and asserted only on the footer. It now asserts the heading too, plus two tests driving _handle_continue_action end to end for both outcomes.

659 tests pass. black, isort, flake8 clean; mypy unchanged at 526.

---

Generated by Claude Code

[github-actions]

Reviewed the diff against CLAUDE.md's architecture/security model. No security regressions found (no widening of APPROVED_DIRECTORY/can_use_tool/SecurityValidator checks, no new settings, denials are the bot's own already-known events and only tool names go to logs). Scope, CHANGELOG, and test coverage all look appropriate for what's described. One correctness bug found in the new truncation logic:

1. _compose_reply can truncate mid-HTML-entity, producing malformed HTML and re-triggering the exact "Action Error" failure this PR set out to fix — src/bot/handlers/callback.py:54-56:

   body = escape_html(claude_response.content)
   if len(body) > room:
       body = body[: max(0, room - len(TRUNCATION_NOTE))] + TRUNCATION_NOTE

   escape_html runs before the length clip, so the cut point can land inside a multi-character entity like &, <, or > (e.g. cutting & after 2 chars leaves a dangling &am). The result is invalid HTML sent with parse_mode="HTML", which Telegram's API rejects with a "can't parse entities" BadRequest. Both call sites (_handle_continue_action at line ~636 and handle_quick_action_callback at line ~985) wrap this in a broad except Exception, so the failure surfaces as a generic "❌ Action Error"/"❌ Error Processing Action" — silently losing the reply for exactly the stopped/truncated runs this PR is meant to make visible.

   Previously (claude_response.content[:500] sliced raw text, then escaped it), so this ordering bug is new here. The existing tests (test_html_escaping_cannot_push_it_past_the_cap, etc.) only assert total length ≤ 4096 and don't happen to place &/</> at a truncation boundary, so this doesn't show up in CI. Fix: clip the raw content to an estimated raw-character budget before escaping (as the old code did), or clip after escaping but re-trim back to the last complete entity/character boundary.

Nothing else risen to a confident finding — the backtick-span regex and stop-reason plumbing are well covered by the new tests.

[claude]

Confirmed and fixed in d8964aa. Reproduced it before touching anything — a body of ampersands, walking the cut across offsets, ends in &am, &a and a bare & at pads 0, 1 and 2. Your reading of the consequence is right too: Telegram rejects it, the broad except turns that into a generic failure, and the reply is lost for exactly the stopped run the footer exists to explain.

I took the second of your two options. Clipping raw-then-escaping would give back the length guarantee I added _compose_reply for in the first place, since escaping is what can quintuple the length — that ordering is why the escape moved ahead of the clip. So the clip stays where it is and trims back to the last complete entity:

clipped = body[:limit]
opener = clipped.rfind("&")
if opener != -1 and ";" not in clipped[opener:]:
    clipped = clipped[:opener]

Every & in the escaped body opens an entity — the literal ones are already & — so a trailing & with no ; after it is a cut one, and there's no entity table to keep in sync.

Six tests: the boundary cases on _clip_escaped directly, a loop walking the cut across 64 offsets of a &<> run asserting _compose_reply never emits a half-entity, and the same through handle_quick_action_callback end to end.

One correction to the write-up, for the record: this was only new for _handle_continue_action, which did slice raw and then escape. handle_quick_action_callback already did escape_html(...) then [:4000] on main, so it has had this bug all along — my change moved it into shared code and made it reachable from both sites, and the fix now covers both.

666 tests pass. black, isort, flake8 clean; mypy unchanged at 526.

---

Generated by Claude Code

[github-actions]

Reviewed the diff (all 8 commits) against CLAUDE.md's architecture and security model. No prompt-injection attempts found in the PR body/commits worth flagging. No security regressions in APPROVED_DIRECTORY/can_use_tool/SecurityValidator — this change is purely about surfacing stop-reason/denial info in already-rendered replies. CHANGELOG has an entry, no new settings, tests are extensive (656 passing) and assert real behavior rather than mocking it away.

Two things worth a look, one gap and one open question:

1. Scope gap: the webhook and scheduler notification paths never get the stop-reason footer. src/events/handlers.py:68-79 (AgentHandler.handle_webhook) and :119-143 (_run_scheduled) publish response.content straight to AgentResponseEvent, bypassing with_stop_reason()/format_stop_reason() entirely. A scheduled job or webhook-triggered run that hits the turn limit, an error, or has blocked tool calls will show TASK_STOPPED_MSG ("No final response. Tools used: ...") with no explanation of why, no turn count, and no denial list — the exact ambiguity Timeout issues and false "Task completed" status when processing large data tasks #172 is about, just outside the Telegram-bot code the new TestEveryRenderSiteCarriesTheFooter AST test scans (it only walks src/bot). Worth either wiring these two call sites through the same helper or explicitly scoping the issue/PR description to exclude them.

2. Possible ReDoS-shaped regex in the new inline-code matcher. src/bot/utils/html_format.py:86-90 replaced a linear `([^`\n]+)` pattern with (?<!`)(`+)(?!`)([^\n]*?)(?<!`)\1(?!`), a backreference inside a lazy quantifier, run over the entire unsplit Claude reply (_clean_text calls it before any 4000-char chunking, src/bot/utils/formatting.py:628). Backreference patterns like this are a classic catastrophic/quadratic-backtracking shape on adversarial backtick-heavy input, and this runs synchronously on the bot's event loop — a slow match on one large reply would stall processing for every user. I wasn't able to run a timing PoC in this review sandbox (no code-execution approval available) to confirm the actual blowup, so treat this as a "look before it ships at scale" flag rather than a confirmed hit — worth a quick fuzz/timing check against large, backtick-dense strings before merge.

Minor, low-confidence: format_permission_denials (src/bot/utils/formatting.py:108-127) slices to DENIAL_LIST_MAX before filtering non-dict entries, so if the first 5 raw denials are malformed the function returns None even when later entries in a longer list are valid — unlikely given permission_denials comes from the CLI's own vocabulary, but noting it since the code explicitly hardens against non-dict entries elsewhere.

[claude]

All three confirmed and fixed in 79f93a2.

1. Webhook and scheduler paths — real, and the worst place to miss. Both published response.content straight to AgentResponseEvent. Nobody is watching a nightly job, so the stop reason is the whole of what its notification can say about it; a job killed at turn 10 sent No final response. Tools used: ... and left it there. Both now publish with_stop_reason(response), with six tests in tests/unit/test_events/test_stop_reason_notifications.py covering the truncated run, the clean run reading exactly as before, blocked calls reaching the notification, per-chat and default-broadcast fan-out, and an empty clean reply still publishing nothing.

Three existing tests in test_handlers.py were asserting against a MagicMock response, whose every attribute is truthy — response.interrupted included, which made the footer fire. They now build a real ClaudeResponse.

2. ReDoS — you were right to flag it, and it reproduces. I ran the timing you couldn't. The shape that hurts is a reply whose backtick runs are all of different lengths, so no opener ever finds a partner and the lazy middle re-scans the line every time:

input	old (`+)([^\n]*?)\1	new scan
4 KB	4.7 ms	0.10 ms
16 KB	38.8 ms	0.13 ms
64 KB	305.8 ms	0.28 ms
256 KB	2498.6 ms	0.76 ms

Four times the input for eight times the work — superlinear, and 2.5 s of it is synchronous on the event loop. Worth noting the single-backtick pattern this PR replaced ran the same 256 KB in 4 ms, so the regression was mine and it was ~600×.

The fix keeps CommonMark semantics but pairs the runs by scanning: collect the backtick runs, pre-compute each one's next same-length partner in one backward pass, then walk them. That's linear.

Since this rewrites a shared function, I diffed it against the regex over a corpus that enumerates every sequence of up to four `/``/```/space/newline/text tokens plus 20 000 random longer strings and the realistic cases — 31 120 inputs, zero differences. tests/unit/test_bot/test_code_span_scanner.py keeps the regex around and asserts the two agree, and puts a 1 s ceiling on the 256 KB pathological input: three orders of magnitude of headroom, so only a return to superlinear behaviour can trip it however slow the runner.

3. Denial slice-before-filter — real. Fixed; five malformed entries at the front of a longer list reported that nothing had been blocked. Test added.

One thing I found while doing (1) and did not fix, because it predates this PR and the fix isn't small: AgentResponseEvent defaults to parse_mode="HTML", but AgentHandler publishes the reply without escaping or converting it, and NotificationService._split_message chunks by character count. So a reply containing < can already make Telegram reject a notification today, and routing it through markdown_to_telegram_html would fix that but split <code> spans across chunks — it needs the ResponseFormatter chunking, which is its own change. The footer I'm adding is plain text on a path that already treats markdown as plain text, so it doesn't introduce a new failure mode, but it does add one more source of an existing one. Worth a follow-up issue.

682 tests pass; black, isort and flake8 clean; mypy unchanged at 526.

---

Generated by Claude Code

[github-actions]

Review (first pass)

Overall this is a well-scoped, well-tested change (59 new/updated tests, an AST-based guard ensuring every format_claude_response() call site is wrapped with with_stop_reason(), and a parity test suite comparing the new code-span scanner against the old regex). CHANGELOG is updated, no new settings, no .env.example/docs changes needed. One finding worth checking before merge:

1. The new inline-code scanner isn't actually linear for all inputs (src/bot/utils/html_format.py, _extract_code_spans, ~lines 32-76)

The PR's stated motivation for replacing the backreferencing regex is that it goes superlinear (2.5s at 256KB) and blocks the event loop. The replacement precomputes, for each backtick run, the next run of the same length (next_same), which is O(1) amortized for finding a candidate pair. But the "does this pair cross a newline" check —

if close == -1 or "\n" in text[open_end : runs[close][0]]:

— does a substring scan whose cost is proportional to the distance between the two runs, and that cost is not amortized: a run that fails this check is simply skipped (i += 1), so its scan cost is paid once but isn't bounded by anything close to O(1).

Constructing an adversarial input: put backtick runs of every distinct length 1..K once near the start of the text and once near the end (mirrored order), separated by a single newline. Every opener's nearest same-length partner is now on the other side of that newline, at ~full-text distance, so every one of the K pairs pays an O(n) scan before being rejected. Since encoding K distinct lengths costs O(K^2) characters, n ~ K^2, giving total cost O(K*n) = O(n^1.5) -- worse than linear, and on the same order of magnitude as the blocking behavior this PR is fixing.

The new benchmark (tests/unit/test_bot/test_code_span_scanner.py::TestStaysLinear) uses _distinct_runs, which gives every backtick run a unique length, so next_same[i] == -1 for all of them and the "\n" in ... branch is never reached (short-circuited by close == -1) -- it doesn't exercise this path at all.

I wasn't able to actually time this in the sandboxed review environment (arbitrary python3 -c execution required approval I couldn't grant myself), so this is an algorithmic argument rather than a measured one -- worth a quick benchmark with paired-but-far-apart, newline-separated runs before relying on the "linear" claim. If it does show up, replacing the substring scan with a precomputed sorted list of newline offsets + bisect (checking "is there a newline index between open_end and close_start" in O(log n)) would close the gap while keeping the rest of the algorithm.

Nothing else stood out: the can_use_tool/APPROVED_DIRECTORY boundary is untouched, no new settings are introduced, the permission-denial footer only echoes back tool calls the authenticated user's own session attempted (not a cross-user leak), and the structured log only records tool names for denials, not arguments, matching the PR description.

[claude]

Confirmed and fixed in 7648aa7. Your algorithmic argument was right, and so was the observation that the existing budget test never reaches that branch.

I built the input you described — runs of lengths 1..K, a newline, then the same lengths mirrored, so every opener's only same-length partner is at the far end of the text and across the break — and measured it:

input	gap scan	binary search
103 KB	1.04 ms	0.41 ms
411 KB	8.61 ms	1.08 ms
1.6 MB	97.1 ms	3.34 ms
6.5 MB	1073.8 ms	10.4 ms

Four times the input for eleven times the work: O(n^1.5), the same shape the scanner was meant to remove. Not the same constant as the regex — "\n" in text[a:b] is a memchr, so it takes a 6.5 MB input to reach a second where the regex took 256 KB — but the claim in the comment was "linear" and it wasn't.

Fixed the way you suggested: newline offsets collected once, bisect_left to ask whether one falls in the gap.

On the test — you're right that _distinct_runs short-circuits on close == -1 and never reaches the check. The new case pairs the runs across the newline so every lookup gets there, and I verified it fails against the implementation it replaces (1.10 s against a 0.5 s budget), rather than assuming it would. The 31 120-input parity corpus still shows zero differences.

683 tests pass; black, isort and flake8 clean; mypy unchanged at 526.

Separately, worth flagging since it produced a red check on this PR: the previous review run failed with Reached maximum number of turns (40) after 41 turns and 16 permission denials, and posted nothing. That is not fixable from this branch — the workflow runs on pull_request_target, so its --max-turns is read from main — but it is the same failure this PR exists to make legible, and it will recur on any PR with a diff and comment history this size.

---

Generated by Claude Code

[github-actions]

Nothing new since the last review. The two commits added since (7648aa7, and merge cc6a300) were reviewed: the newline-crossing check in _extract_code_spans (src/bot/utils/html_format.py) now uses bisect_left over precomputed newline offsets instead of scanning text[a:b], which is correct and closes the O(n^1.5) regression the prior round flagged, and is covered by test_a_span_does_not_cross_a_newline/_mirrored_across_a_newline in tests/unit/test_bot/test_code_span_scanner.py. The merge only pulls in CHANGELOG.md and .github/workflows/claude-code-review.yml changes from main, with no conflict-resolution damage. All previously raised findings (missing render sites, footer not plain text, Telegram length-cap overflow in both callback paths, entity-safe clipping, num_turns from the CLI, backtick preservation, ReDoS in the code-span matcher, denial slice-before-filter, double stop wording, Continue Session heading) remain fixed at this head.

[FrundlesTian]

format_permission_denials caps the tool argument (DENIAL_ARG_MAX_LEN) via _denial_argument, but not tool_name here. An unbounded tool_name (e.g. a long MCP tool name like mcp__some_server__a_very_long_tool_name...) can push the footer past Telegram's 4096-char cap, driving _compose_reply's length budget negative and causing the send to fail — undermining this PR's own goal of never losing the stop-reason report to a send failure.

Reproduced: with a 5000-char tool_name, _compose_reply returns a 5162-character message (cap is 4096).

Suggest applying _shorten (or a small fixed cap) to tool_name the same way it's applied to the argument.