feat: add /model command for runtime model switching via Telegram

[bOsowski]

Summary

• Add /model command for per-user runtime model switching
• Thread model_override through orchestrator → facade → SDK integration
• Extract actual model from API response (AssistantMessage.model)
• 14 new tests covering command behavior, parameter passthrough, and model override priority

Related Issue

Fixes #138

Type of Change

• New feature

Testing

• Tests added/updated
• All tests pass
• Manual testing completed

Checklist

• Code follows project style guidelines
• Self-review completed
• No breaking changes

[FridayOpenClawBot]

PR Review
Reviewed head: 2c2a1b376b27385e4296a0452a0b82897fb1e5f0

Summary

• Adds /model command for per-user runtime model switching with alias support, default reset, and propagation through the facade → SDK chain. Clean layered approach with good test coverage (~345 new test lines across 3 files).

What looks good

• Correct propagation path: orchestrator → facade → _execute → SDK options. No shortcuts.
• ClaudeResponse.model populated from first AssistantMessage — good for surfacing what actually ran.
• _resolve_model_display is a clean pure helper; easy to test and reason about.
• Test coverage is solid given the scope.
• Handler and BotCommand registration both present.

Issues / questions

1. [Blocker] src/bot/orchestrator.py — Audit logging is missing. CLAUDE.md explicitly requires audit logging when adding agentic commands. Neither the /model set nor the reset path logs the action (user ID, selected model, timestamp). Other commands presumably do this. This needs to be added before merge.

2. [Bug] src/bot/orchestrator.py — last_model not updated after _handle_agentic_media_message. agentic_text and agentic_document both update context.user_data["last_model"] from claude_response.model, but _handle_agentic_media_message doesn't — so /model (no args) will show a stale or missing model after a voice/photo message. Either update last_model in the media handler (or its caller), or document this as intentional.

3. [Question] src/claude/sdk_integration.py — Aliases are passed raw to the SDK. /model sonnet sets model_override = "sonnet" and that string goes directly into ClaudeAgentOptions(model=...). Does the SDK accept short aliases, or does it require full model names like claude-sonnet-4-6? If the SDK rejects aliases silently (falls back to default) or raises an error, users get confusing behaviour. Either resolve aliases to canonical names in agentic_model before storing, or document the SDK's behaviour here.

4. [Question] src/bot/orchestrator.py — In-memory only, no persistence. Model override lives in context.user_data which is lost on bot restart. The PR body doesn't mention this. Is this a known limitation or an oversight? Worth a comment in the code and/or a note in the /model help text so users aren't surprised after a restart.

5. [Minor] src/bot/orchestrator.py — No input validation on model name. Any arbitrary string (including whitespace-only after .strip(), or very long strings) gets accepted and stored. A basic sanity check (non-empty after strip, reasonable length, optional allowlist or prefix check) would prevent user confusion and potential SDK errors.

Suggested tests (if needed)

• /model <alias> → verify the raw alias string is what reaches ClaudeAgentOptions.model (documents the current behaviour explicitly so a future alias-resolution change is caught)
• Bot restart simulation: set model override, reinitialise context.user_data, verify override is gone — confirms the in-memory behaviour is tested, not assumed
• Media message flow: send media after /model sonnet, verify last_model in user_data is (or isn't) updated — documents the current gap

Verdict

• ⚠️ Merge after fixes — the missing audit log is a stated project requirement, and the last_model gap in the media handler is a real inconsistency. The alias-passthrough and persistence questions are lower risk but should be answered before this ships to avoid user-facing surprises.

— Friday, AI assistant to @RichardAtCT

[FridayOpenClawBot]

PR Review
Reviewed head: beba83893a4eaf3df9eafa9df52401333a8f0a5c

Summary

• Adds /model [model_name] command for per-user runtime model switching, stored in context.user_data["model_override"]
• Threads the override through orchestrator → facade → SDK integration with a clean, non-leaky layering
• Captures the actual model used from AssistantMessage.model in the response and stores it as last_model for display purposes
• 14 new tests across orchestrator, facade, and SDK integration layers

What looks good

• Clean layering — model_override passes through the stack without spreading concerns; each layer just forwards it
• _resolve_model_display is a pure utility that's easy to test in isolation and handles all three states cleanly
• escape_html applied correctly to the user-supplied model name — no XSS risk via HTML parse mode
• Audit logging on model changes is the right call
• getattr(msg, "model", None) is a safe fallback in case the SDK field is absent
• Test coverage is solid — command display, set, reset, passthrough to run_command, model detection from response, and rejection of oversized names are all covered

Issues / questions

1. [Important] src/bot/orchestrator.py — No validation that the model name is a valid Claude identifier. A user can set model_override to any arbitrary string (up to 100 chars — e.g. gpt-4, lol, an empty string after .strip()). The SDK will fail at runtime with a potentially cryptic error rather than at set-time, leaving the user confused about why messages stop working. Consider either: (a) maintaining a loose allowlist of known claude-* prefixes and warning on mismatch, or (b) at minimum responding with a clear error if the next request fails due to an invalid model. Silent misconfiguration is poor UX.

2. [Important] src/bot/orchestrator.py — model_name can technically be the empty string after .strip() if the input is /model (one space). The length check len(model_name) > 100 doesn't catch this — the existing not model_name check does, but only after .strip(). This is fine as-is, but worth confirming that the not model_name guard reliably covers the post-strip empty case. It does — just noting it so the test for this path is explicit. There's a test for len > 100 but I don't see one for the whitespace-only input ("/model ").

3. [Important] src/bot/orchestrator.py — Audit log is placed outside the if/else block, so /model default (the reset path) logs args=["default"] rather than something semantically distinguishable from setting a model literally named "default". This makes audit log queries for "who changed their model to X" noisier. Suggest logging args=["<reset>"] or args=[] on the reset path, or restructuring the audit call to be inside each branch.

4. [Nit] src/bot/orchestrator.py — update.message could be None in edge cases (channel posts, certain forwarded messages). The early access update.message.text.split() would raise AttributeError. Other handlers may already guard this — if there's a middleware guarantee that message is non-null for registered handlers, a comment noting that would remove the ambiguity.

5. [Nit] src/bot/orchestrator.py — get_bot_commands return type is list (unparameterised). CLAUDE.md requires mypy strict / disallow_untyped_defs. This method predates the PR, but the PR adds to it — worth updating the signature to list[BotCommand] while here.

6. [Nit] src/claude/sdk_integration.py — response_model is only set from the first AssistantMessage encountered (guarded by if not response_model). If the actual model is only known after the ResultMessage processes (loop order dependency), this could silently return None. Not a bug given current SDK behaviour, but the comment in the code (# Extract cost, tools, session_id, and model from result message) says "from result message" yet the model actually comes from AssistantMessage — a clarifying comment would help future readers.

Suggested tests (if needed)

• /model (whitespace-only arg) → should reply "Invalid model name." (not currently explicit in tests)
• Back-to-back /model opus then /model default then query /model → should show server/default model, not opus

Verdict
⚠️ Merge after fixes — the lack of model name validation (#1) is the main concern for UX; the audit log semantic issue (#3) is low-risk but easy to fix. The rest are nits.

— Friday, AI assistant to @RichardAtCT

[bOsowski]

Issues / questions

1. [Important] src/bot/orchestrator.py — No validation that the model name is a valid Claude identifier. A user can set model_override to any arbitrary string (up to 100 chars — e.g. gpt-4, lol, an empty string after .strip()). The SDK will fail at runtime with a potentially cryptic error rather than at set-time, leaving the user confused about why messages stop working. Consider either: (a) maintaining a loose allowlist of known claude-* prefixes and warning on mismatch, or (b) at minimum responding with a clear error if the next request fails due to an invalid model. Silent misconfiguration is poor UX.

This is handled - the response when an invalid model is selected is clear.

2. [Important] src/bot/orchestrator.py — model_name can technically be the empty string after .strip() if the input is /model (one space). The length check len(model_name) > 100 doesn't catch this — the existing not model_name check does, but only after .strip(). This is fine as-is, but worth confirming that the not model_name guard reliably covers the post-strip empty case. It does — just noting it so the test for this path is explicit. There's a test for len > 100 but I don't see one for the whitespace-only input ("/model ").

Telegram trims the command, such that the "/model " becomes just "/model". This is reflected in the UI and it will trigger the base "/model" command.

3. [Important] src/bot/orchestrator.py — Audit log is placed outside the if/else block, so /model default (the reset path) logs args=["default"] rather than something semantically distinguishable from setting a model literally named "default". This makes audit log queries for "who changed their model to X" noisier. Suggest logging args=["<reset>"] or args=[] on the reset path, or restructuring the audit call to be inside each branch.

Will address in the next PR.

[FridayOpenClawBot]

PR Review (incremental)
Reviewed head: 32f345324fb9c263d721874e45f939f2dc705534
(Previously reviewed: beba83893a4eaf3df9eafa9df52401333a8f0a5c)

Changes in this update

• Moved audit_logger lookup before the if/else branch
• Reset path now logs command="model_reset" with args=[] instead of command="model" with args=["default"]
• Set path unchanged: still logs command="model" with args=[model_name]
• New test test_agentic_model_reset_audit_logged verifies reset semantics end-to-end

Assessment

• ✅ Issue 🚀 Complete Advanced Features Implementation #3 fully addressed — the audit log is no longer outside the if/else, and the reset path is now semantically distinct and queryable
• ✅ Implementation is correct; audit_logger is resolved once before the branch, which is cleaner and avoids the double lookup from the old code
• ✅ Test is solid: pre-populates model_override so the pop() actually exercises the removal, asserts called_once_with (guards against duplicate logging), and checks all three kwargs
• Minor: no test verifying that audit_logger.log_command is NOT called when audit_logger is absent on the reset path — but the existing pattern elsewhere in the test suite likely covers this implicitly, so not a blocker

Remaining open issues from prior review

• [Important] Feature: Migrate to Python SDK #1 — No validation that model_name is a valid Claude model identifier (arbitrary strings still accepted)
• [Important] Feature: Migrate to Python SDK with Flexible Authentication #2 — No test for whitespace-only input (e.g. /model )
• [Nit] "Loading .env file Error #4 — update.message could be None; unguarded .reply_text calls remain
• [Nit] Conversation Context Not Maintained Between Messages in Telegram Integration #5 — get_bot_commands return type still not parameterised
• [Nit] Fix conversation context loss between Telegram messages #6 — Stale comment in sdk_integration.py

Verdict
⚠️ Merge after fixes — the two [Important] items (#1 model validation, #2 whitespace test) from the prior review are still open and should be addressed before merge. This commit itself is correct and well-tested.

— Friday, AI assistant to @RichardAtCT