PR Review
Reviewed head: b6aecdda92e50a371e4862230e5cb06a2f126ee9

Summary

• Adds /restart command that sends a confirmation message then raises SIGTERM so systemd (or any Restart=always process manager) brings the bot back up
• Fixes a pre-existing bug where set_my_commands() was called before the PTB HTTP client was initialized, so the command menu was never actually pushed to Telegram
• Registered in both agentic (6→6+1) and classic (13→14) handler modes

What looks good

• The SIGTERM approach is correct — it hooks into the existing graceful-shutdown path in main.py
• The HTTP client init fix (await self.app.initialize() before _set_bot_commands()) is a genuine bug fix with a clear explanation; the idempotency note about PTB's _initialized flag is accurate
• Test updated to mock app.initialize as AsyncMock, keeping the test suite clean

Issues / questions

1. [Important] src/bot/handlers/command.py — restart_command has no authorisation check. Any user who can message the bot can trigger a restart. If this bot is deployed with ALLOWED_USERS, the existing auth middleware should cover it — but if the handler is somehow reachable without going through the middleware (e.g. a different handler registration path), any user could DoS the bot. Worth confirming the middleware always wraps this handler.
2. [Nit] The confirmation message says "Back in a few seconds" but there's no guarantee systemd will restart immediately (e.g. if RestartSec is set). "Back shortly" would be safer wording.

Suggested tests

• Unit test that restart_command calls os.kill(os.getpid(), signal.SIGTERM) (mock os.kill)
• Unit test that an unauthorized user cannot trigger /restart (if auth middleware can be tested in isolation)

Verdict
⚠️ Merge after fixes — the auth concern is worth a quick confirmation before this ships. Everything else looks good.

— Friday, AI assistant to @RichardAtCT

Thanks for the review! Pushed a fix commit addressing both points:

1. Auth concern (confirmed safe):
The auth middleware runs at handler group -2 and raises ApplicationHandlerStop for unauthenticated users — this blocks all handlers at group 10 (where /restart lives) before they execute. There is no registration path that bypasses this middleware chain. Added a docstring note to restart_command documenting this explicitly.

2. Wording nit:
Changed "Back in a few seconds" → "Back shortly" since RestartSec / process manager timing isn't guaranteed.

3. Suggested test added:
Added test_restart_command_sends_sigterm — mocks os.kill and verifies it's called with (os.getpid(), signal.SIGTERM), plus checks the confirmation message is sent.

Unit test that an unauthorized user cannot trigger /restart

This is already covered by the existing middleware test suite (test_auth_rejection_raises_handler_stop, test_real_auth_middleware_rejection in test_middleware.py) which proves that any unauthenticated update is blocked before reaching any handler. No per-command auth test is needed.

PR Review
Reviewed head: 5b9c15b084ec14967f452523e92960cfb13f8c73

Summary

• Adds /restart command that sends a confirmation message then SIGTERMs the bot process, relying on systemd (or equivalent) to restart it
• Fixes a real bug: set_my_commands() was called before the PTB Application's HTTP client was initialized, so the command menu was never actually pushed to Telegram
• Registered in both agentic and classic modes with full test coverage

What looks good

• The bug fix is well-explained and the solution (await self.app.initialize() before _set_bot_commands(), relying on PTB's idempotent _initialized flag) is clean and correct
• The test for restart_command properly patches os.kill and verifies both the SIGTERM call and the confirmation message
• Auth protection via the existing middleware group (-2) is a sensible choice — no per-handler duplication needed

Issues / questions

1. [Important] src/bot/handlers/command.py (restart_command) — os.kill(os.getpid(), signal.SIGTERM) is called after await update.message.reply_text(...). If the SIGTERM handler is aggressive (e.g., closes the event loop immediately), there's a small window where the reply might not have been fully sent/flushed before shutdown begins. In practice PTB's graceful shutdown should handle this, but it's worth verifying that the reply is actually delivered before the process exits. Consider adding a short asyncio.sleep(0) or await asyncio.sleep(0.1) to yield control and let PTB flush the outgoing queue before sending SIGTERM.

2. [Important] src/bot/handlers/command.py (restart_command) — There's no rate limiting or cooldown on /restart. Any authorized user can hammer it rapidly. If you have multiple authorized users, this could cause a rapid restart loop. A simple in-memory debounce (e.g., reject if a restart was triggered in the last 10s) would be prudent.

3. [Nit] src/bot/handlers/command.py — import os and import signal are added at the top of the file. Fine, but worth noting that these imports now make command.py slightly less portable to non-POSIX environments (Windows). Presumably not a concern for this bot, but worth a comment.

Suggested tests (if needed)

• Test verifying the confirmation reply is sent before os.kill is called (ordering assertion on mock call sequence)

Verdict
⚠️ Merge after fixes — the SIGTERM timing issue is worth addressing. The bug fix for command menu initialization is genuinely valuable and correct.

— Friday, AI assistant to @RichardAtCT

PR Review
Reviewed head: 5b9c15b084ec14967f452523e92960cfb13f8c73

Summary

• Adds /restart command that sends SIGTERM to the bot process, relying on systemd Restart=always to bring it back up
• Fixes a pre-existing bug where set_my_commands() was called before the HTTPX client was initialized, causing the command menu to never update
• Registers the command in both agentic and classic modes; tests updated accordingly

What looks good

• The SIGTERM approach is clean — reuses the existing graceful-shutdown path rather than inventing a new one; idempotent double-init is correctly handled by ptb's _initialized guard
• The bug fix in core.py is well-reasoned and the comment explains the "why" clearly
• Good test coverage: unit test patches os.kill to verify the signal is sent, and orchestrator tests verify command counts

Issues / questions

1. [Important] src/bot/handlers/command.py — No authorization check is mentioned in the handler itself; the docstring says auth is handled by group -2 middleware. This is fine if restart_command is always registered at group 10 and the middleware is unconditional. But the handler could be called directly in tests or future code without the middleware in place — worth a quick assert or a comment that auth is assumed, not a per-handler check.
2. [Nit] src/bot/handlers/command.py — The SIGTERM is sent before the bot can process any further messages; if the reply_text awaitable hasn't fully flushed to Telegram's servers, the confirmation message might be lost. Consider a tiny asyncio.sleep(0.5) or waiting for the message to be delivered before signalling — though in practice ptb flushes inline, so this is low risk.

Suggested tests (if needed)

• Consider an integration-level test (or at least a documented runbook) for the systemd restart cycle, since that's the critical path and can't be unit-tested easily.

Verdict
⚠️ Merge after fixes — the auth concern is low risk given the current setup, but worth a short comment in code; everything else is solid.

— Friday, AI assistant to @RichardAtCT