PR Review
Reviewed head: a887f0e125cfe3c31dd29c9a8314ac4a2931a0ab

Summary

• Adds a native Telegram inline "Stop" button to every Claude progress message, letting users interrupt long-running requests without sending a text command.
• Implements StopAwareUpdateProcessor — a custom PTB update processor that lets stop callbacks bypass the sequential lock so they can fire while a handler is blocked awaiting run_command().
• Refactors agentic_text, agentic_document, and agentic_photo into a shared _process_text_with_claude() to eliminate ~250 lines of duplication.

What looks good

• The StopAwareUpdateProcessor design is clean and well-reasoned. The concurrent-bypass approach solves the PTB sequential-lock problem elegantly without touching PTB internals beyond the documented BaseUpdateProcessor API.
• Excellent test coverage: 647 lines of tests covering owner/non-owner blocking, double-stop prevention, cleanup on success/error, SDK interrupt wiring, and sequential/bypass concurrency — this is the right level of rigor for a feature touching async flow control.
• interrupt_task.cancel() in the finally block is correct — no leaked tasks when the SDK finishes before the user hits Stop.

Issues / questions

1. [Important] src/bot/orchestrator.py — reply_to_message_id removed from all reply_text calls in _process_text_with_claude. The original agentic_text passed reply_to_message_id=(update.message.message_id if i == 0 else None) for the first response chunk, so users could see which message was being answered. This looks like an unintentional regression from the refactor. Was this dropped on purpose?

2. [Important] src/bot/update_processor.py — asyncio.Lock() created in __init__ (sync context). This is fine in Python ≥ 3.10 (locks no longer bind to a loop at creation), but if the project still targets 3.9 it could surface subtle loop-mismatch bugs on some platforms. Worth a comment or a version pin check.

3. [Nit] src/bot/orchestrator.py — _handle_stop_callback: query.data.split(":", 1)[1] and int(...) will raise IndexError/ValueError if the callback data is malformed. Given the pattern=r"^stop:" handler filter guarantees the prefix, this is very low risk, but a try/except or explicit validation would be belt-and-suspenders.

4. [Nit] src/bot/update_processor.py — max_concurrent_updates=256 is a magic number with a comment explaining it but no config hook. Fine for now, but a named constant (_MAX_CONCURRENT = 256) would make it easier to find if PTB's default semaphore ever needs revisiting.

Suggested tests (if needed)

• Consider a test asserting that a second call to _process_text_with_claude for the same user_id (e.g. from a document) correctly overwrites _active_requests[user_id] without leaving a stale entry. The sequential lock makes this safe today, but the test would lock in the assumption.

Verdict
⚠️ Merge after fixes — the reply_to_message_id regression (#1) is worth confirming as intentional before merging; everything else is solid.

— Friday, AI assistant to @RichardAtCT

PR Review
Reviewed head: 6126a1e8e624a16bc4127ca1794b4b30eb5e2541

Summary

• Adds a native Telegram inline "Stop" button to every Claude progress message, letting users cancel long-running requests mid-flight.
• Introduces StopAwareUpdateProcessor to bypass PTB's sequential lock for stop callbacks, and ActiveRequest dataclass to track per-user in-flight requests.
• Refactors agentic_text, agentic_document, and agentic_photo into a shared _process_text_with_claude() to avoid duplicating stop logic.

What looks good

• The StopAwareUpdateProcessor design is clean — using an asyncio.Lock for sequential updates while letting stop callbacks run concurrently is the right approach, and the tests verify the ordering correctly.
• Consolidating the three handlers into _process_text_with_claude() removes ~250 lines of duplication without losing any functionality; the audit_command param is a nice touch.
• Test coverage is solid: the concurrency tests in test_stop_button.py and test_update_processor.py cover the meaningful edge cases.

Issues / questions

1. [Important] src/bot/orchestrator.py — _active_requests is a plain Dict[int, ActiveRequest] on the orchestrator instance. If the bot serves multiple users simultaneously, there's a race between a user's new request overwriting their old ActiveRequest in the dict and the old request's finally-block trying to pop it. Scenario: user fires request A → _active_requests[uid] = req_A → user fires request B before A resolves (shouldn't normally happen with the sequential lock, but the lock is per-instance, not per-user) → _active_requests[uid] = req_B → A's finally pops req_B, leaving a dangling active request. The sequential lock prevents two requests for the same user running concurrently only if they come in order — worth adding a comment confirming this is the intended invariant, or guarding with if self._active_requests.get(user_id) is active_request.

2. [Important] src/bot/orchestrator.py — _handle_stop_callback parses user_id directly from query.data (int(query.data.split(":", 1)[1])). Malformed callback data (e.g. stop:notanint) will raise an unhandled ValueError that propagates through PTB's error handling. A try/except ValueError around the parse with a silent await query.answer() + return would be safer.

3. [Important] src/bot/orchestrator.py _process_text_with_claude — the progress_msg delete in the try block is now missing. The old code had await progress_msg.delete() after the happy-path response was sent; the refactored version removed this (the finally only pops _active_requests). The progress message (with the Stop button) will linger after a successful response unless something else deletes it. Check whether progress_msg.delete() still happens on the success path — if not, users will see a stale "Working..." message.

4. [Nit] src/bot/update_processor.py — max_concurrent_updates=256 is a magic number with a comment but no constant. Something like _MAX_CONCURRENT = 256 # high enough to never block stop callbacks at module level would make it self-documenting.

Suggested tests (if needed)

• A test for the malformed stop:notanint callback data to confirm it's handled gracefully (issue 2).
• A test confirming the progress message is deleted (or edited to reply_markup=None) after a successful Claude response (issue 3).

Verdict
⚠️ Merge after fixes — issues 2 and 3 are real user-facing bugs; issue 1 is low-risk given the sequential lock but worth a comment.

— Friday, AI assistant to @RichardAtCT

PR Review
Reviewed head: 4b2da3e3d945a6381a5b695b60d4430ccb9fe536

Summary

• Adds a native Telegram inline "Stop" button to every Claude progress message, letting users cancel long-running requests without issuing a command.
• Introduces StopAwareUpdateProcessor to bypass PTB's sequential update lock for stop:* callbacks, so the interrupt fires even while a handler holds the lock.
• Refactors agentic_document and agentic_photo into a shared _process_text_with_claude() to avoid duplicating stop logic.

What looks good

• StopAwareUpdateProcessor is elegant — the dual-path lock design (bypass for stop, sequential for everything else) cleanly solves the concurrency problem without rearchitecting the whole bot.
• The interrupt watcher task + cancel() in the finally block is correct; no dangling tasks.
• Test coverage is solid: concurrency ordering, double-stop, non-owner guard, and partial-response preservation are all exercised.

Issues / questions

1. [Important] src/bot/orchestrator.py — _active_requests is a plain Dict[int, ActiveRequest] on the orchestrator instance. If the bot handles multiple users simultaneously (which StopAwareUpdateProcessor now explicitly allows for stop callbacks), two concurrent _process_text_with_claude calls for the same user_id will overwrite each other's entry. The second assignment stomps the first, making the original request un-interruptable and leaking the old ActiveRequest. Is it intentional to allow only one active request per user? If so, consider explicitly rejecting/cancelling the second rather than silently overwriting.

2. [Important] src/bot/orchestrator.py _handle_stop_callback — TOCTOU: reads active.interrupted, then sets it. Two rapid stop taps could both pass the guard before either sets it. Simpler fix: check active.interrupt_event.is_set() instead of the separate interrupted bool — asyncio.Event.is_set() is already idempotent and atomic within the event loop.

3. [Nit] src/bot/update_processor.py — _MAX_CONCURRENT = 256 — since stop callbacks bypass _sequential_lock anyway, this semaphore only applies to regular updates. Worth a comment clarifying that real concurrency control is _sequential_lock, not the semaphore cap.

Suggested tests

• A test for concurrent same-user requests (issue Feature: Migrate to Python SDK #1): assert the second is either rejected or the first is properly cleaned up before the overwrite.

Verdict
⚠️ Merge after fixes — both [Important] items are genuine concurrency correctness issues; both have simple fixes.

— Friday, AI assistant to @RichardAtCT

PR Review

Overall: Clean, well-structured implementation. The StopAwareUpdateProcessor approach is the right call for PTB's sequential-update model — bypassing the lock only for stop callbacks avoids a whole class of deadlock bugs. The refactor into _process_text_with_claude() is a real improvement, eliminating ~200 lines of duplication.

Observations & Questions

stop:{user_id} callback data pattern
Keying on user_id rather than a random token means a user could theoretically replay an old callback button press from a previous message to stop a new request. It's a minor UX edge case (Telegram caches inline buttons), but worth noting. A random nonce per request stored in ActiveRequest would be more precise.

_active_requests overwrites on concurrent sends
If the same user sends two messages before the first completes, _active_requests[user_id] is overwritten. The old request's active_request reference in the finally block still has the guard if self._active_requests.get(user_id) is active_request, so cleanup is safe — but the first request loses its stop button functionality silently. Could be worth either queueing or rejecting the second message while one is active.

claude_response.interrupted attribute
The review code references claude_response.interrupted — this doesn't appear in the diff. Is it a newly added field on the response object, or an existing one? If it's new, it's not visible here.

StopAwareUpdateProcessor import inside initialize()
Minor style nit — the import is local (from .update_processor import ...) inside the method body rather than at module top. Intentional to avoid circular imports? A brief comment would help.

Photo error handling improvement
The photo handler now uses reply_text on error instead of progress_msg.edit_text (since progress_msg no longer exists there). Good fix — but the error branch discards any exception and returns, which means _process_text_with_claude is never called. Looks correct, just worth a quick read to confirm the user sees the error message before the early return.

Tests

Good to see test_update_processor.py covering the concurrency behaviour. Does the test suite cover the _handle_stop_callback path (e.g. wrong user, already-complete, double-stop)?

Verdict: Solid PR. Main things worth addressing before merge: the interrupted attribute source, and whether concurrent-sends-same-user needs handling. Everything else is minor.

— Friday, AI assistant to @RichardAtCT

Hey! This is ready to merge but has conflicts with main after recent merges (voice transcription #106 and streaming drafts #123 both touched overlapping files). Could you rebase against main? Should be straightforward — the conflicts are in orchestrator.py, features.py, and settings.py where both sides added new features at the same spots. Thanks!

Hey! This is ready to merge but has conflicts with main after recent merges (voice transcription #106 and streaming drafts #123 both touched overlapping files). Could you rebase against main? Should be straightforward — the conflicts are in orchestrator.py, features.py, and settings.py where both sides added new features at the same spots. Thanks!

Yes, sure, will do!

@RichardAtCT fixed merge conflicts