feat(mcp): auto-register MCP tools as first-class + RAG-aware prompt

[Smilez1985]

Summary

Builds on #12 (minimal MCP-SSE client). When RAG_TRANSPORT=mcp and the server is reachable at startup, discovers the server's advertised tools via tools/list and registers each one as a first-class entry in the LLM's TOOLS schema list and TOOL_MAP dispatcher. The agent calls e.g. rag_search(query=..., top_k=3) directly instead of going through the two-hop mcp_list_tools → mcp_call_tool indirection.

A small system-prompt section "External Memory (MCP)" is emitted only when at least one MCP tool was registered. It lists what's available and tells the bot to:

• search RAG BEFORE answering questions about user preferences, project rules, decisions, or past context;
• persist durable lessons via the persist tool;
• optionally announce session context once per conversation.

Why

The previous PR exposed mcp_list_tools / mcp_call_tool as generic glue, but the LLM wouldn't reach for them on its own — and even when it did, the two-hop indirection wasted turns. Auto-registration lets the agent treat the RAG as its durable memory the same way it already uses remember_fact / recall_facts for the in-process store; the prompt block tells it when to use them. Without the prompt block I observed the bot answering from short-term memory and missing rules that were sitting in the RAG.

Behavior

• Idempotent: a tool whose name collides with an existing built-in is skipped.
• Graceful: server unreachable at start → silent no-op, mcp_list_tools / mcp_call_tool fallback path remains usable; the bot stays alive.
• Activation: gated by RAG_TRANSPORT=mcp (same env var as feat(mcp): bot as MCP client over SSE — minimal stdlib client + 2 LLM tools #12). Default rest deployments are unaffected — the directive is only added when registration succeeds.

Test plan

• Syntax check both files
• Smoke test: 7 tools discovered against a real MCP server, all wired into TOOL_MAP
• Direct call TOOL_MAP['rag_search'](query='display variant', top_k=2) returns real hits
• Bot service restart with the change in place: stays active, no traceback, prompt block visible in built system context
• End-to-end via Telegram: ask the bot a question whose answer requires RAG content, observe tool call rag_search in logs

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Auto-update with backup and rollback capabilities
  • UPS HAT battery monitoring and /battery command
  • RAG integration with /rag command for memory and search
  • Model selection interface via /model command
  • Ollama LLM support with tool capability detection
  • Multi-language support with language-pinned prompts
  • MCP protocol integration for external tool discovery and execution
  • E-paper display variant support for mono and 3-color panels

• Bug Fixes

  • Fixed onboarding completion detection logic
  • Fixed language injection into system prompts
  • Fixed error message localization

• Documentation

  • Updated CHANGELOG with new feature documentation
  • Enhanced display hardware variant documentation

[Smilez1985]

Quick context on this integration — wanted to be upfront about where it comes from.

The RAG backend I built against is a project I call rag-core: a self-hosted, project-scoped retrieval memory layer with a REST API and an MCP-SSE gateway (Qdrant-backed, multi-collection, frontmatter-aware chunking, designed for AI agents). The repo is currently private while I finish a handful of pre-public-readiness items, but the plan is to open it up before long.

This PR series (#10, #12, #13, #14) is intentionally written against a generic contract — any RAG backend that speaks the documented REST shape, and any MCP-SSE server with tools/list + tools/call. Nothing here pins to rag-core specifically; it just happens to be the implementation I run.

Reason for sending this upstream rather than keeping it on my fork: I use OpenClawGotchi as one of my main development drivers, and the RAG integration has become part of how I work with it day-to-day. Carrying a long-lived branch and re-rebasing on every upstream pull is a fair amount of churn — having it on main would save a meaningful amount of repeat work on my side.

No pressure on accepting; happy to iterate to whatever shape fits best, or to wait if you'd rather see the other end of the wire first. Once rag-core is public I'll ping back here — at that point you'd be welcome to clone the repo and stand up your own instance if you want to see how it pairs with the bot from the other side. (Just to be clear: this isn't an invite to my running instance — it's an invite to the source, so you can run your own.)

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds five major interconnected capabilities: external RAG service integration via REST and MCP-over-SSE clients, Ollama LLM backend support with interactive model selection, UPS HAT battery monitoring via I2C, E-Paper display multi-variant support (mono vs 3-color), and an auto-update system with backup/rollback. Language localization, improved onboarding detection, and comprehensive tool wiring complete the feature set.

Changes

Core Feature Additions

Layer / File(s)	Summary
Configuration, Dependencies & Infrastructure Setup src/config.py, requirements.txt, .gitignore, setup.sh	Adds Ollama (OLLAMA_MODEL, OLLAMA_API_BASE), RAG (RAG_API_URL, RAG_API_KEY, RAG_DEFAULT_COLLECTIONS), and Ollama preset wiring; includes optional smbus2 dependency for battery monitoring; gitignore entries for data/active_model.json and backups/; sudoers rule for passwordless systemctl restart.
Auto-Update System with Backup & Rollback scripts/auto_update.sh, src/bot/handlers.py, src/main.py	New bash auto-update script with git fetch/pull, optional pre-update tar.gz backup of gotchi.db/.env/data/ with pruning, venv dependency refresh on requirements.txt change, systemd service restart with verification, and auto-rollback on failure with distinct exit codes; owner-only /update command with --check mode support.
Battery Hardware Monitoring & Display Integration src/hardware/battery.py, src/hardware/auto_mood.py, src/hardware/system.py, src/bot/handlers.py, src/ui/gotchi_ui.py	New BatteryReading dataclass with INA219-over-I2C reading and voltage-to-percentage mapping; is_available() / read() functions with graceful degradation; system stats multi-line display with optional battery section; status text shortening to avoid duplication with header metrics; /battery command handler; footer battery rendering (center-aligned, red accent on B-variant low-battery state).
E-Paper Display Multi-Variant Support (Mono vs 3-Color B) src/drivers/epd2in13b_V4.py, src/hardware/display.py, src/ui/gotchi_ui.py, gotchi-skills/display/SKILL.md	New 3-color B-variant driver supporting separate black/red buffers and refresh semantics; variant-driven display timing/refresh constants; update deduplication, debouncing, and periodic full-refresh for ghosting mitigation on mono panels; subprocess environment variable propagation for display/GPIO context; dynamic driver selection via OCG_DISPLAY_VARIANT (mono/b/auto); expanded hardware/color protocol documentation.
Ollama LLM Backend & Model Selection Picker src/bot/handlers.py, src/config.py, src/main.py	Inline /model command with preset list and Ollama submenu; async Ollama HTTP probing for installed models and tool-call capability detection; /use handler now triggers show_face() for visual feedback; callback-based model switching with persistent selection.
RAG REST & MCP-over-SSE Client Infrastructure src/llm/rag_client.py, src/llm/rag_mcp_client.py	REST client for external RAG service with /query, /ingest-text, and /health endpoints; automatic disable on missing RAG_API_URL; Bearer auth when RAG_API_KEY set; MCP-over-SSE client using background thread SSE reader for event/message dispatch, synchronous JSON-RPC request/response, handshake/tool discovery, and singleton lifecycle management.
RAG Tool Wiring & Command Handler src/llm/litellm_connector.py, src/llm/prompts.py, src/bot/handlers.py	New query_rag, persist_to_rag, mcp_list_tools, mcp_call_tool tool functions with configuration gating and input validation; import-time MCP tool auto-registration; system prompt directives for language pinning and external memory guidance; /rag command handler supporting health/config display and formatted query results.
Language Localization & Onboarding Completion src/bot/heartbeat.py, src/hardware/display.py, src/bot/onboarding.py	Language pinning in heartbeat prompt to force non-English model output when BOT_LANGUAGE is set; localized error_screen() SAY/mood by error category (rate limit, timeout, auth, parsing, generic); mtime-based onboarding completion (auto-delete BOOTSTRAP.md when IDENTITY.md is newer).
Main Bot Application Wiring & HTTP Configuration src/main.py	Handler imports for model/update/battery/RAG commands and callbacks; explicit HTTP timeout settings (read_timeout, write_timeout, connect_timeout, pool_timeout) for long-polling reliability; Telegram command/callback route registration for model, rag, cron, jobs, update, battery and `model
Release Documentation CHANGELOG.md	Unreleased entry documenting new features (RAG REST/MCP, Ollama, model picker, auto-update with backup/rollback, battery monitoring, display variants), config changes (HTTP timeouts), fixes (language injection, error localization, onboarding completion), and implementation notes (sync MCP, SSE routing, graceful no-op behavior).

🎯 4 (Complex) | ⏱️ ~60 minutes

🐰 Five new features hop in: RAG remembers, Ollama learns, batteries charge, displays split—mono meets three-color—and updates roll back safely. The bot grows smarter, more colorful, and ready to roll! ✨🔋🎨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 61.11% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat(mcp): auto-register MCP tools as first-class + RAG-aware prompt' directly and clearly summarizes the main changes: MCP tool auto-registration as first-class LLM tools and the addition of RAG-aware system prompts.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

⚔️ Resolve merge conflicts

• Resolve merge conflict in branch feat/mcp-tool-auto-registration

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[turmyshevd]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (2)

CHANGELOG.md (2)

31-34: 💤 Low value

Consider expanding graceful degradation note to cover auto-registration.

The Notes section mentions graceful degradation for unreachable MCP servers, but per the PR objectives, auto-registration also has graceful behavior: it's idempotent (skips registration if a tool name collides with an existing built-in), and failures are logged but non-fatal. This characteristic isn't captured in line 34.

📋 Suggested enhancement

 ### Notes
 - The MCP client uses a sync API throughout — slots into the existing TOOL_MAP dispatcher without async plumbing.
 - A single background thread reads the SSE stream and routes JSON-RPC responses by id; one connected client is reused per process via a lazy singleton.
-- Graceful degradation: when the MCP server is unreachable or `RAG_TRANSPORT` isn't set, the new tools return informative no-op strings; the bot stays alive.
+- Graceful degradation: when the MCP server is unreachable or `RAG_TRANSPORT` isn't set, the generic tools return informative no-op strings and auto-registration is a silent no-op; the bot stays alive. Auto-registration is idempotent and skips tool names that collide with existing built-ins.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CHANGELOG.md` around lines 31 - 34, Update the "Notes" section to explicitly
mention graceful degradation for auto-registration: state that the
auto-registration process for MCP tools is idempotent (it skips registering when
a tool name collides with built-ins) and that registration failures are logged
but non-fatal; reference the existing terms used in the diff (MCP,
RAG_TRANSPORT, TOOL_MAP) so readers understand this behavior applies alongside
the unreachable-server/no-op behavior already described.

---

9-9: 💤 Low value

Consider removing transient implementation detail.

The "~250 LoC" line count is an implementation detail that may change with future edits and doesn't provide actionable information to users reading the changelog.

✂️ Suggested simplification

-- **Bot can also act as a generic MCP client over SSE.** New module `src/llm/rag_mcp_client.py` is a hand-rolled minimal MCP-over-SSE client (~250 LoC) using only the stdlib + `requests` (already in the venv via litellm). Speaks just enough of the MCP spec to do `initialize` + `tools/list` + `tools/call` against any SSE-transport server — no `mcp[cli]` dependency, so the Pi Zero 2W's RAM budget is respected (the official package pulls in `cryptography`, `pydantic-settings`, `starlette`, `uvicorn`, etc.).
+- **Bot can also act as a generic MCP client over SSE.** New module `src/llm/rag_mcp_client.py` is a hand-rolled minimal MCP-over-SSE client using only the stdlib + `requests` (already in the venv via litellm). Speaks just enough of the MCP spec to do `initialize` + `tools/list` + `tools/call` against any SSE-transport server — no `mcp[cli]` dependency, so the Pi Zero 2W's RAM budget is respected (the official package pulls in `cryptography`, `pydantic-settings`, `starlette`, `uvicorn`, etc.).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CHANGELOG.md` at line 9, Remove the transient implementation detail "~250
LoC" from the CHANGELOG entry describing the new module; edit the sentence that
mentions `src/llm/rag_mcp_client.py` to omit the line-count parenthetical so it
reads as a concise feature description (keep mention that it is a minimal
hand-rolled MCP-over-SSE client using stdlib + `requests` but drop the volatile
LoC figure).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Around line 7-11: The changelog omits documentation of the MCP
auto-registration feature: when RAG_TRANSPORT=mcp and the MCP server is
reachable at startup the code calls tools/list and auto-registers each
discovered tool into TOOL_MAP so they become first-class callable tools (not
just via mcp_list_tools → mcp_call_tool), and it also emits an "External Memory
(MCP)" system prompt when at least one MCP tool is registered; update the Added
section to mention auto-registration of discovered tools into TOOL_MAP, give an
example (e.g., discovered tool becomes rag_search(query..., top_k=...)), and
note the emitted "External Memory (MCP)" system prompt behavior when MCP tools
are present (include references to mcp_list_tools, mcp_call_tool, TOOL_MAP, and
RAG_TRANSPORT).

In `@src/bot/handlers.py`:
- Around line 1078-1081: The check-only branch in the handler currently treats
any non-zero proc.returncode as "Up-to-date"; update the logic in the check_only
block (where proc.returncode and msg.edit_text are used) to map returncode==0 =>
"🆕 Updates available", returncode==1 => "✅ Up-to-date", and any other non-zero
returncode => "❌ Update check failed (exit X)" (include the actual
proc.returncode in the message) before calling msg.edit_text to display the
status and the captured output variable out.
- Around line 1053-1056: The current guard uses get_admin_id() alone so when it
returns None the check passes and non-owners can run /update; change the logic
in the /update handler to always fetch the owner id (e.g. call get_owner_id())
and enforce that the caller must be the owner unless an admin is explicitly
configured and matches the caller: compute owner_id = get_owner_id() and
admin_id = get_admin_id(), then if user_id != owner_id and (admin_id is None or
user_id != admin_id) call update.message.reply_text("⛔ Owner-only command.") and
return; update references to user_id, get_admin_id, get_owner_id, and
update.message.reply_text in the handler accordingly.

In `@src/bot/heartbeat.py`:
- Around line 315-317: The language-pin added in heartbeat.py currently tells
the model to translate the entire reply, which can localize control tokens like
"FACE:" and "SAY:" and break parse_and_execute_commands and mood routing; change
the prompt text used where the string is built (the language pin around
_lang_name) to explicitly exclude or preserve protocol tokens by instructing
"Translate all user-visible content into {_lang_name}; do NOT translate, alter,
or localize any protocol/control tokens such as FACE:, SAY:, or other command
lines — leave them exactly as-is" (apply the same change to the similar string
at the other occurrence around lines 322-324). Ensure the wording makes control
tokens invariant so parse_and_execute_commands and mood routing receive English
tokens unchanged.

In `@src/bot/onboarding.py`:
- Around line 27-35: The except clause around the filesystem checks for
identity_file and BOOTSTRAP_FILE is too broad; narrow it to only handle
filesystem-related errors by catching OSError (and its subclasses) instead of
Exception. Update the try/except in the onboarding auto-complete block that uses
identity_file.stat(), identity_file.exists(), and BOOTSTRAP_FILE.stat() so it
only catches OSError, and keep the existing log.warning message (including the
exception) for that case; let other exceptions propagate.

In `@src/drivers/epd2in13b_V4.py`:
- Around line 157-159: The fallback currently returns a buffer filled with 0x00
(black) despite the comment saying "blank buffer"; change the return to a white
(blank) buffer by returning bytes filled with 0xFF instead of 0x00 while keeping
the same size calculation using self.width and self.height (i.e.,
(int(self.width/8) * self.height)); update the return in the method that
contains logger.warning("Wrong image dimensions...") so a dimension mismatch
yields a white screen rather than black.
- Around line 79-83: The busy() loop can hang forever if BUSY stays asserted;
add a timeout to busy() (e.g., parameter timeout_ms or module constant
BUSY_TIMEOUT_MS) and track start time (use time.monotonic()); inside the
while(epdconfig.digital_read(self.busy_pin) != 0) loop check elapsed time and if
it exceeds the timeout call epdconfig.delay_ms as before, then log an error
including the timeout and raise a TimeoutError (or return a failure boolean) so
callers know the display never released; ensure time is imported and update any
callers if you change the signature.

In `@src/hardware/display.py`:
- Around line 23-25: The current variant detection sets _VARIANT_B true when
_DISPLAY_VARIANT is "auto", which incorrectly applies B-specific timing/debounce
before the driver resolves; change the condition that defines _VARIANT_B so it
only returns True for explicit B identifiers (e.g., "b", "epd2in13b", "3color",
"tricolor") and excludes "auto" (leave "auto" to be resolved later by the
driver), i.e., update the logic around _DISPLAY_VARIANT and _VARIANT_B to not
treat "auto" as a B variant and ensure any later driver-resolution code can set
the effective variant when known.

In `@src/llm/litellm_connector.py`:
- Line 625: The current user-facing error strings include the raw RAG_API_URL
environment value (os.environ.get('RAG_API_URL')) which can leak secrets; update
both places that return that string to instead return a generic message like
"Error: RAG service unreachable" and move the detailed URL (and any exception)
to internal logs (use the module logger, e.g., logger.error or logger.debug) so
the URL is recorded for debugging but not exposed to users. Ensure you replace
direct interpolations of os.environ.get('RAG_API_URL') in the two return
statements with the generic text and log the sensitive value and exception
context via the logger in the surrounding function(s).
- Around line 1337-1353: The loop currently trusts server-provided tool
descriptors and mutates globals (TOOLS, TOOL_MAP, _TOOL_ICONS,
_MCP_REGISTERED_TOOLS) before validating them; update the registration to first
validate the tool metadata (ensure name is a non-empty string matching allowable
characters, not already in TOOL_MAP, description length <=1024, and inputSchema
is a dict resembling a JSON Schema with at least a "type" key or an object with
"properties"), then attempt to create the wrapper via _make_mcp_tool_wrapper
inside a try/except and only append to TOOLS, set TOOL_MAP[name],
_TOOL_ICONS.setdefault(name, ...), and _MCP_REGISTERED_TOOLS.append(name) after
validation and successful wrapper creation; on validation or wrapper failure
skip the tool and optionally log the reason rather than mutating globals.

In `@src/llm/prompts.py`:
- Around line 55-60: The prompt currently hardcodes tool names `rag_search`,
`rag_persist`, and `rag_session_announce`, which can point to non-existent
tools; update the prompt generation in prompts.py to reference the actual tool
names from the runtime `tools` mapping or to use placeholders populated at build
time (e.g., inject tools['rag_search'] / tools['rag_persist'] or a formatted
`{rag_search_tool}`) instead of literal strings so the directive always matches
available tool identifiers; modify the prompt template lines that mention "call
`rag_search`", "call `rag_persist`", and "If `rag_session_announce` exists" to
use the derived names/placeholder variables and ensure fallback text handles
missing tools gracefully.

In `@src/llm/rag_client.py`:
- Around line 93-96: The current request dict coercion uses int(top_k) directly
which can raise ValueError/TypeError; wrap the conversion of top_k in a
try/except (catching ValueError and TypeError), set a safe default (e.g.,
default_top_k = 5 or RAG_DEFAULT_TOP_K if available) when conversion fails, then
apply max(1, min(converted_top_k, 50)) before inserting into the dict; reference
the top_k variable and the place where the request dict is constructed to locate
and replace the direct int(top_k) usage.

In `@src/llm/rag_mcp_client.py`:
- Around line 90-91: The _responses and _response_events maps are accessed and
mutated from multiple threads without synchronization; add a dedicated
threading.Lock (e.g., self._responses_lock) in the class and use it to guard all
writes/reads to self._responses and self._response_events (creation, deletion,
lookup, and signaling), and when iterating for SSE cleanup take a snapshot
(e.g., acquire lock, copy keys or items, release lock) before iterating to avoid
races and dropped wakeups; update every place that touches
_responses/_response_events (including the SSE cleanup path and the blocks
around the sections noted) to use the lock.
- Around line 305-312: If MCPSSEClient.connect() can succeed but initialize()
may fail leaving background resources, ensure the partially-created client is
closed before returning; in the block that creates the client (MCPSSEClient,
client.connect(), client.initialize(), and setting _singleton), on any exception
call a cleanup on the temporary client (e.g. if hasattr(client, "close") call
client.close(), else if hasattr(client, "disconnect") call client.disconnect(),
or call a generic shutdown method) before returning None, and do not set
_singleton unless initialize() fully succeeded.

---

Nitpick comments:
In `@CHANGELOG.md`:
- Around line 31-34: Update the "Notes" section to explicitly mention graceful
degradation for auto-registration: state that the auto-registration process for
MCP tools is idempotent (it skips registering when a tool name collides with
built-ins) and that registration failures are logged but non-fatal; reference
the existing terms used in the diff (MCP, RAG_TRANSPORT, TOOL_MAP) so readers
understand this behavior applies alongside the unreachable-server/no-op behavior
already described.
- Line 9: Remove the transient implementation detail "~250 LoC" from the
CHANGELOG entry describing the new module; edit the sentence that mentions
`src/llm/rag_mcp_client.py` to omit the line-count parenthetical so it reads as
a concise feature description (keep mention that it is a minimal hand-rolled
MCP-over-SSE client using stdlib + `requests` but drop the volatile LoC figure).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6078972e-4e95-41a3-9776-d8d5a4d06198

📥 Commits

Reviewing files that changed from the base of the PR and between 4757b2f and cd631b0.

📒 Files selected for processing (21)

• .gitignore
• CHANGELOG.md
• gotchi-skills/display/SKILL.md
• requirements.txt
• scripts/auto_update.sh
• setup.sh
• src/bot/handlers.py
• src/bot/heartbeat.py
• src/bot/onboarding.py
• src/config.py
• src/drivers/epd2in13b_V4.py
• src/hardware/auto_mood.py
• src/hardware/battery.py
• src/hardware/display.py
• src/hardware/system.py
• src/llm/litellm_connector.py
• src/llm/prompts.py
• src/llm/rag_client.py
• src/llm/rag_mcp_client.py
• src/main.py
• src/ui/gotchi_ui.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Document the MCP auto-registration feature.

The changelog describes the two generic tools (mcp_list_tools, mcp_call_tool) but omits the auto-registration capability that is the main point of this PR. According to the PR objectives and test plan, when RAG_TRANSPORT=mcp and the MCP server is reachable at startup, the code discovers advertised tools via tools/list and registers each as a first-class entry in TOOL_MAP, allowing the LLM to call them directly (e.g., rag_search(query=..., top_k=3)) instead of the two-step mcp_list_tools → mcp_call_tool flow.

Additionally, the changelog doesn't mention the "External Memory (MCP)" system prompt section that is emitted when at least one MCP tool is registered, which instructs the agent when and how to use RAG tools.

📝 Suggested addition to document auto-registration

Consider adding a bullet point after line 11:

 - New env var `RAG_TRANSPORT=rest|mcp` (default `rest`). When `mcp` is selected, `RAG_API_URL` is interpreted as the MCP-SSE base URL (e.g. `http://your-rag-host:8766`).
+- **MCP tool auto-registration**: When `RAG_TRANSPORT=mcp` and the MCP server is reachable at startup, discovered tools (via `tools/list`) are automatically registered as first-class entries in `TOOL_MAP`. The LLM can call them directly (e.g., `rag_search(query, top_k)`) instead of using the generic two-step `mcp_list_tools` → `mcp_call_tool` flow. Colliding names are skipped; failures are gracefully logged. A "External Memory (MCP)" system prompt section is injected when registration succeeds, guiding the agent to search RAG before answering questions about preferences, project rules, or past context, and to persist durable lessons.
 - **`/model` Telegram command**: inline-keyboard model picker. Without args it opens buttons for every preset (gemini, glm, ollama). With an argument (`/model glm`) it falls through to the existing `/use` flow. `/use` and `/switch` remain as text aliases.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CHANGELOG.md` around lines 7 - 11, The changelog omits documentation of the
MCP auto-registration feature: when RAG_TRANSPORT=mcp and the MCP server is
reachable at startup the code calls tools/list and auto-registers each
discovered tool into TOOL_MAP so they become first-class callable tools (not
just via mcp_list_tools → mcp_call_tool), and it also emits an "External Memory
(MCP)" system prompt when at least one MCP tool is registered; update the Added
section to mention auto-registration of discovered tools into TOOL_MAP, give an
example (e.g., discovered tool becomes rag_search(query..., top_k=...)), and
note the emitted "External Memory (MCP)" system prompt behavior when MCP tools
are present (include references to mcp_list_tools, mcp_call_tool, TOOL_MAP, and
RAG_TRANSPORT).

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce owner-only /update even when admin is not configured.

Current check is bypassed when get_admin_id() returns None, which can expose /update to non-owner users in permissive configs.

Proposed fix

     admin_id = get_admin_id()
-    if admin_id and user_id != admin_id:
+    if admin_id is None:
+        await update.message.reply_text("⛔ Owner is not configured. Set `ALLOWED_USERS` first.")
+        return
+    if user_id != admin_id:
         await update.message.reply_text("⛔ Owner-only command.")
         return

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	admin_id = get_admin_id()
	if admin_id and user_id != admin_id:
	await update.message.reply_text("⛔ Owner-only command.")
	return
	admin_id = get_admin_id()
	if admin_id is None:
	await update.message.reply_text("⛔ Owner is not configured. Set `ALLOWED_USERS` first.")
	return
	if user_id != admin_id:
	await update.message.reply_text("⛔ Owner-only command.")
	return

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bot/handlers.py` around lines 1053 - 1056, The current guard uses
get_admin_id() alone so when it returns None the check passes and non-owners can
run /update; change the logic in the /update handler to always fetch the owner
id (e.g. call get_owner_id()) and enforce that the caller must be the owner
unless an admin is explicitly configured and matches the caller: compute
owner_id = get_owner_id() and admin_id = get_admin_id(), then if user_id !=
owner_id and (admin_id is None or user_id != admin_id) call
update.message.reply_text("⛔ Owner-only command.") and return; update references
to user_id, get_admin_id, get_owner_id, and update.message.reply_text in the
handler accordingly.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

/update check currently masks failures as “up-to-date”.

In check mode, any non-zero exit code is treated as up-to-date. Exit codes like 2/3/4 should be surfaced as errors, not success.

Proposed fix

         if check_only:
-            status = "🆕 Updates available" if proc.returncode == 0 else "✅ Up-to-date"
-            await msg.edit_text(f"{status}\n\n```\n{out}\n```", parse_mode="Markdown")
+            if proc.returncode == 0:
+                status = "🆕 Updates available"
+            elif proc.returncode == 1:
+                status = "✅ Up-to-date"
+            else:
+                status = f"❌ Update check failed (exit {proc.returncode})"
+            await msg.edit_text(f"{status}\n\n```\n{out}\n```", parse_mode="Markdown")
             return

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bot/handlers.py` around lines 1078 - 1081, The check-only branch in the
handler currently treats any non-zero proc.returncode as "Up-to-date"; update
the logic in the check_only block (where proc.returncode and msg.edit_text are
used) to map returncode==0 => "🆕 Updates available", returncode==1 => "✅
Up-to-date", and any other non-zero returncode => "❌ Update check failed (exit
X)" (include the actual proc.returncode in the message) before calling
msg.edit_text to display the status and the captured output variable out.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Language pin currently conflicts with command protocol tokens.

These instructions can make the model localize FACE:/SAY: control lines, which breaks parse_and_execute_commands expectations and mood routing.

Proposed fix

-            f"Schreibe **die GESAMTE Antwort** auf {_lang_name}. Jedes Wort, jeder "
-            f"Satz, auch die SAY:- und FACE:-Zeilen am Ende. Die Vorlage unten ist "
+            f"Schreibe die Antwort auf {_lang_name}. "
+            f"Behalte die Steuer-Tokens **FACE:**, **SAY:** und **STATUS:** exakt so bei "
+            f"(nicht übersetzen). Übersetze nur den freien Textinhalt. Die Vorlage unten ist "
             f"nur auf Englisch, weil sie eine Systemanweisung ist — DEINE Antwort "
             f"muss auf {_lang_name} sein.\n\n---\n\n"
@@
-            f"\n\n[Reflektiere auf {_lang_name}. Antworte auf {_lang_name}. "
-            f"Nicht auf Englisch. {_lang_name} only.]"
+            f"\n\n[Reflektiere auf {_lang_name}. Tokens FACE:/SAY:/STATUS: bleiben unverändert.]"
         )

Also applies to: 322-324

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bot/heartbeat.py` around lines 315 - 317, The language-pin added in
heartbeat.py currently tells the model to translate the entire reply, which can
localize control tokens like "FACE:" and "SAY:" and break
parse_and_execute_commands and mood routing; change the prompt text used where
the string is built (the language pin around _lang_name) to explicitly exclude
or preserve protocol tokens by instructing "Translate all user-visible content
into {_lang_name}; do NOT translate, alter, or localize any protocol/control
tokens such as FACE:, SAY:, or other command lines — leave them exactly as-is"
(apply the same change to the similar string at the other occurrence around
lines 322-324). Ensure the wording makes control tokens invariant so
parse_and_execute_commands and mood routing receive English tokens unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify broad exception usage in onboarding-related code.
rg -n -C3 'except\s+Exception\b' src/bot/onboarding.py src/bot

Repository: turmyshevd/openclawgotchi

Length of output: 12939

---

🏁 Script executed:

cat -n src/bot/onboarding.py | sed -n '20,40p'

Repository: turmyshevd/openclawgotchi

Length of output: 1218

---

🏁 Script executed:

head -20 src/bot/onboarding.py

Repository: turmyshevd/openclawgotchi

Length of output: 577

---

Narrow the broad exception catch in onboarding auto-complete.

At line 34, except Exception is too broad and can mask non-filesystem bugs; only filesystem-related exceptions should be handled here. All operations in the try block are pathlib.Path methods that raise OSError or its subclasses.

Proposed fix

         try:
             if identity_file.exists() and (
                 identity_file.stat().st_mtime > BOOTSTRAP_FILE.stat().st_mtime
             ):
                 log.info("Auto-completing onboarding: IDENTITY.md newer than BOOTSTRAP.md")
                 BOOTSTRAP_FILE.unlink()
                 return False
-        except Exception as e:
+        except OSError as e:
             log.warning(f"Onboarding auto-complete check failed: {e}")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	try:
	if identity_file.exists() and (
	identity_file.stat().st_mtime > BOOTSTRAP_FILE.stat().st_mtime
	):
	log.info("Auto-completing onboarding: IDENTITY.md newer than BOOTSTRAP.md")
	BOOTSTRAP_FILE.unlink()
	return False
	except Exception as e:
	log.warning(f"Onboarding auto-complete check failed: {e}")
	try:
	if identity_file.exists() and (
	identity_file.stat().st_mtime > BOOTSTRAP_FILE.stat().st_mtime
	):
	log.info("Auto-completing onboarding: IDENTITY.md newer than BOOTSTRAP.md")
	BOOTSTRAP_FILE.unlink()
	return False
	except OSError as e:
	log.warning(f"Onboarding auto-complete check failed: {e}")

🧰 Tools

🪛 Ruff (0.15.12)

[warning] 34-34: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bot/onboarding.py` around lines 27 - 35, The except clause around the
filesystem checks for identity_file and BOOTSTRAP_FILE is too broad; narrow it
to only handle filesystem-related errors by catching OSError (and its
subclasses) instead of Exception. Update the try/except in the onboarding
auto-complete block that uses identity_file.stat(), identity_file.exists(), and
BOOTSTRAP_FILE.stat() so it only catches OSError, and keep the existing
log.warning message (including the exception) for that case; let other
exceptions propagate.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate MCP tool metadata before mutating TOOLS and TOOL_MAP.

The registration path trusts server-provided name/inputSchema as-is. A malformed descriptor can poison the global tool schema and break tool-calling for the whole session.

🛡️ Suggested fix

     registered = 0
     for tool in tools:
-        name = tool.get("name")
-        if not name or name in TOOL_MAP:
+        name = tool.get("name")
+        if not isinstance(name, str):
+            continue
+        name = name.strip()
+        if not name or name in TOOL_MAP:
             continue
         desc = (tool.get("description") or name)[:1024]
-        schema = tool.get("inputSchema") or {"type": "object", "properties": {}}
+        schema = tool.get("inputSchema")
+        if not isinstance(schema, dict):
+            schema = {"type": "object", "properties": {}}
+        elif schema.get("type") not in (None, "object"):
+            log.warning("MCP auto-registration: skipping %s (non-object input schema)", name)
+            continue
+        schema.setdefault("type", "object")
+        schema.setdefault("properties", {})
         TOOLS.append({
             "type": "function",
             "function": {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/llm/litellm_connector.py` around lines 1337 - 1353, The loop currently
trusts server-provided tool descriptors and mutates globals (TOOLS, TOOL_MAP,
_TOOL_ICONS, _MCP_REGISTERED_TOOLS) before validating them; update the
registration to first validate the tool metadata (ensure name is a non-empty
string matching allowable characters, not already in TOOL_MAP, description
length <=1024, and inputSchema is a dict resembling a JSON Schema with at least
a "type" key or an object with "properties"), then attempt to create the wrapper
via _make_mcp_tool_wrapper inside a try/except and only append to TOOLS, set
TOOL_MAP[name], _TOOL_ICONS.setdefault(name, ...), and
_MCP_REGISTERED_TOOLS.append(name) after validation and successful wrapper
creation; on validation or wrapper failure skip the tool and optionally log the
reason rather than mutating globals.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t hardcode rag_search / rag_persist in a dynamic MCP-tool prompt.

The directive can instruct calls to non-existent tools when server names differ, even though you already have the real names in tools.

♻️ Suggested fix

-    return (
+    search_tool = next((t for t in tools if any(k in t.lower() for k in ("search", "recall", "query"))), None)
+    persist_tool = next((t for t in tools if any(k in t.lower() for k in ("persist", "save", "write", "store"))), None)
+    session_tool = next((t for t in tools if "session" in t.lower() and "announce" in t.lower()), None)
+
+    search_instr = (
+        f"call `{search_tool}` (or the equivalent recall tool)"
+        if search_tool else
+        "call the appropriate memory-search tool"
+    )
+    persist_instr = (
+        f"call `{persist_tool}` to save it"
+        if persist_tool else
+        "call the appropriate memory-persist tool to save it"
+    )
+
+    return (
         "\n---\n## External Memory (MCP)\n"
         f"You have first-class access to a long-term RAG memory via these tools: {bullet_list}.\n"
         "Treat the RAG as your durable memory — your in-process facts and chat history are short-term.\n"
         "Use it proactively, not just when the user asks:\n"
         "- BEFORE answering questions about user preferences, project rules, decisions, or past context: "
-        "call `rag_search` (or the equivalent recall tool) to ground your reply in stored knowledge. "
+        f"{search_instr} to ground your reply in stored knowledge. "
         "Do not assume from memory alone if a relevant rule might exist.\n"
         "- WHEN you learn something durable (preferences, decisions, project facts, hard-won lessons): "
-        "call `rag_persist` to save it. Skip casual chat. Prefer short, factual notes with tags.\n"
-        "- If `rag_session_announce` exists, you may announce session context once per conversation "
+        f"{persist_instr}. Skip casual chat. Prefer short, factual notes with tags.\n"
+        f"- If `{session_tool}` exists, you may announce session context once per conversation "
         "so the server can route you to the right collections.\n"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/llm/prompts.py` around lines 55 - 60, The prompt currently hardcodes tool
names `rag_search`, `rag_persist`, and `rag_session_announce`, which can point
to non-existent tools; update the prompt generation in prompts.py to reference
the actual tool names from the runtime `tools` mapping or to use placeholders
populated at build time (e.g., inject tools['rag_search'] / tools['rag_persist']
or a formatted `{rag_search_tool}`) instead of literal strings so the directive
always matches available tool identifiers; modify the prompt template lines that
mention "call `rag_search`", "call `rag_persist`", and "If
`rag_session_announce` exists" to use the derived names/placeholder variables
and ensure fallback text handles missing tools gracefully.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard top_k coercion to prevent runtime exceptions.

int(top_k) can raise ValueError/TypeError (e.g., malformed tool args), currently uncaught.

Proposed fix

-    body = {
+    try:
+        safe_top_k = max(1, min(int(top_k), 50))
+    except (TypeError, ValueError):
+        safe_top_k = 5
+
+    body = {
         "query": text.strip()[:8192],
         "collections": collections or RAG_DEFAULT_COLLECTIONS,
-        "top_k": max(1, min(int(top_k), 50)),
+        "top_k": safe_top_k,
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/llm/rag_client.py` around lines 93 - 96, The current request dict
coercion uses int(top_k) directly which can raise ValueError/TypeError; wrap the
conversion of top_k in a try/except (catching ValueError and TypeError), set a
safe default (e.g., default_top_k = 5 or RAG_DEFAULT_TOP_K if available) when
conversion fails, then apply max(1, min(converted_top_k, 50)) before inserting
into the dict; reference the top_k variable and the place where the request dict
is constructed to locate and replace the direct int(top_k) usage.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Synchronize cross-thread response maps to avoid races.

_responses / _response_events are mutated from multiple threads without locking. This can drop wakeups or throw during iteration in the SSE cleanup path.

Proposed fix (lock shared maps + snapshot iteration)

         self._responses: dict[int, dict] = {}
         self._response_events: dict[int, threading.Event] = {}
+        self._responses_lock = threading.Lock()
@@
-            for evt in self._response_events.values():
+            with self._responses_lock:
+                pending = list(self._response_events.values())
+            for evt in pending:
                 evt.set()
@@
-            if msg_id in self._response_events:
-                self._responses[msg_id] = msg
-                self._response_events[msg_id].set()
+            with self._responses_lock:
+                evt = self._response_events.get(msg_id)
+                if evt is not None:
+                    self._responses[msg_id] = msg
+                    evt.set()
@@
-        self._response_events[req_id] = evt
+        with self._responses_lock:
+            self._response_events[req_id] = evt
@@
-            resp = self._responses.pop(req_id, None)
+            with self._responses_lock:
+                resp = self._responses.pop(req_id, None)
@@
-            self._response_events.pop(req_id, None)
+            with self._responses_lock:
+                self._response_events.pop(req_id, None)

Also applies to: 150-173, 183-204

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/llm/rag_mcp_client.py` around lines 90 - 91, The _responses and
_response_events maps are accessed and mutated from multiple threads without
synchronization; add a dedicated threading.Lock (e.g., self._responses_lock) in
the class and use it to guard all writes/reads to self._responses and
self._response_events (creation, deletion, lookup, and signaling), and when
iterating for SSE cleanup take a snapshot (e.g., acquire lock, copy keys or
items, release lock) before iterating to avoid races and dropped wakeups; update
every place that touches _responses/_response_events (including the SSE cleanup
path and the blocks around the sections noted) to use the lock.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Close partially created client when singleton setup fails.

If connect() succeeds but initialize() fails, the temporary client is not closed, leaving resources/background thread alive.

Proposed fix

         if _singleton is None:
+            client = None
             try:
                 client = MCPSSEClient(base, api_key=api_key)
                 client.connect()
                 client.initialize()
                 _singleton = client
             except Exception as e:
+                if client is not None:
+                    try:
+                        client.close()
+                    except Exception:
+                        pass
                 log.warning(f"MCP client setup failed ({base}): {e}")
                 return None

🧰 Tools

🪛 Ruff (0.15.12)

[warning] 310-310: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/llm/rag_mcp_client.py` around lines 305 - 312, If MCPSSEClient.connect()
can succeed but initialize() may fail leaving background resources, ensure the
partially-created client is closed before returning; in the block that creates
the client (MCPSSEClient, client.connect(), client.initialize(), and setting
_singleton), on any exception call a cleanup on the temporary client (e.g. if
hasattr(client, "close") call client.close(), else if hasattr(client,
"disconnect") call client.disconnect(), or call a generic shutdown method)
before returning None, and do not set _singleton unless initialize() fully
succeeded.