Skip to content

feat: expand LLM provider support to include OpenRouter and NVIDIA NIM#67

Open
MauroDruwel wants to merge 7 commits intomemovai:mainfrom
MauroDruwel:main
Open

feat: expand LLM provider support to include OpenRouter and NVIDIA NIM#67
MauroDruwel wants to merge 7 commits intomemovai:mainfrom
MauroDruwel:main

Conversation

@MauroDruwel
Copy link
Copy Markdown

@MauroDruwel MauroDruwel commented Feb 19, 2026

Add OpenRouter & NVIDIA NIM LLM Providers

Summary

Add OpenRouter and NVIDIA NIM (completely free) as LLM providers alongside Anthropic and OpenAI.

Both expose OpenAI-compatible chat-completions endpoints, reusing existing OpenAI request/response logic with no new serialization code needed.


Architecture

Function Anthropic OpenAI OpenRouter NVIDIA NIM
llm_api_url() api.anthropic.com/v1/messages api.openai.com/v1/chat/completions openrouter.ai/api/v1/chat/completions integrate.api.nvidia.com/v1/chat/completions
llm_api_host() api.anthropic.com api.openai.com openrouter.ai integrate.api.nvidia.com
llm_api_path() /v1/messages /v1/chat/completions /v1/chat/completions /v1/chat/completions

Usage

OpenRouter

# Set provider
set_model_provider openrouter

# Set API key (from openrouter.ai/keys)
set_api_key <your-openrouter-api-key>

# Set model
set_model <model-name>

NVIDIA NIM (free)

# Set provider
set_model_provider nvidia_nim

# Set API key (from org.ngc.nvidia.com/setup/api-keys)
set_api_key <your-nim-api-key>

# Set model
set_model <model-name>

Available Models

OpenRouter

  • anthropic/claude-sonnet-4.6
  • anthropic/claude-opus-4.6
  • openai/gpt-5.2-pro
  • openai/gpt-5.2
  • openai/gpt-5.1
  • google/gemini-3-pro-preview
  • google/gemini-3-flash-preview
  • qwen/qwen3.5-397b-a17b
  • deepseek/deepseek-v3.2
  • x-ai/grok-4.1-fast
  • 100+ additional models - see https://openrouter.ai/models

Free models:

NVIDIA NIM (Completely Free)

  • qwen/qwen3.5-397b-a17b
  • z-ai/glm5
  • nvidia/embedding
  • minimaxai/minimax-m2.1
  • stepfun-ai/step-3.5-flash
  • moonshotai/kimi-2.5
  • 100+ additional models - see https://build.nvidia.com

Get an API key: https://org.ngc.nvidia.com/setup/api-keys


What Changed

File Change
main/mimi_config.h Add MIMI_OPENROUTER_API_URL and MIMI_NVIDIA_NIM_API_URL
main/llm/llm_proxy.c Add provider_is_openrouter() / provider_is_nvidia_nim() and update routing
main/llm/llm_proxy.h Update provider docstring
main/cli/serial_cli.c Update CLI help text
main/mimi_secrets.h.example Update secrets template
README.md Update documentation
docs/ARCHITECTURE.md Update architecture docs

Test Plan

  • Project compiles successfully with idf.py build
  • set_model_provider openrouter is accepted via serial CLI
  • set_model_provider nvidia_nim is accepted via serial CLI
  • set_api_key correctly stores the OpenRouter/NVIDIA NIM API key in NVS
  • Messages are successfully sent to and responses received from the OpenRouter/NVIDIA NIM API
  • Tool calling functions correctly via the OpenRouter/NVIDIA NIM endpoint
  • Switching provider back to anthropic or openai continues to work without regression

Summary by CodeRabbit

  • New Features

    • Expanded LLM provider support: Anthropic, OpenAI, OpenRouter, and NVIDIA NIM can be selected and used interchangeably via CLI/config, with unified model/provider switching and tool support.
  • Documentation

    • Updated README, architecture, quick-start, and CLI help to reflect multi-provider support, generalized API key guidance, and consistent tool-calling across all supported providers.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 19, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds OpenRouter and NVIDIA NIM as supported LLM providers alongside Anthropic and OpenAI; updates docs, config macros, CLI help text, and the LLM proxy to resolve provider-specific hosts/paths/URLs and choose appropriate authorization and request/response formats.

Changes

Cohort / File(s) Summary
Documentation
README.md, docs/ARCHITECTURE.md
Expanded provider list and examples; replaced Claude/Anthropic-centric references with a unified LLM provider abstraction and updated architecture/data-flow diagrams and descriptions to include OpenRouter and NVIDIA NIM.
Configuration & Secrets
main/mimi_config.h, main/mimi_secrets.h.example
Added MIMI_OPENROUTER_API_URL and MIMI_NVIDIA_NIM_API_URL macros; generalized secrets example comment to refer to multiple LLM providers.
CLI help text
main/cli/serial_cli.c
Updated set_model_provider help string to list `(anthropic
LLM proxy & integration
main/llm/llm_proxy.c, main/llm/llm_proxy.h
Added detection helpers for OpenRouter/NVIDIA; introduced provider_is_openai_compat; provider-specific URL/host/path resolution; unified auth header handling (Bearer for OpenAI-compatible, x-api-key otherwise); adjusted request-body fields and response parsing/tool-call extraction for OpenAI-compatible vs legacy providers.

Sequence Diagram(s)

sequenceDiagram
    participant User as "User"
    participant CLI as "CLI"
    participant LLMProxy as "LLM Proxy"
    participant Provider as "LLM Provider\n(Anthropic / OpenAI / OpenRouter / NVIDIA)"

    User->>CLI: submit chat or set provider
    CLI->>LLMProxy: forward request + provider selection
    LLMProxy->>LLMProxy: resolve URL/host/path/auth (provider_is_openai_compat?)
    alt Tool invocation required
        LLMProxy->>LLMProxy: build tool call payload (provider-specific fields)
        LLMProxy->>Provider: POST (Bearer or x-api-key) to resolved path
    else No tools
        LLMProxy->>Provider: POST (Bearer or x-api-key) to resolved path
    end
    Provider-->>LLMProxy: response (messages / tokens / tool outputs)
    LLMProxy-->>CLI: normalized response
    CLI-->>User: display result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • IRONICBo

Poem

🐰 I hopped through code and README lines,
Added routes where bright API shines.
OpenRouter, NVIDIA, Anthropic too,
OpenAI joins the meadow view.
One proxy, four doors — a joyful chew!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title directly and clearly summarizes the main change: expanding LLM provider support to include OpenRouter and NVIDIA NIM, which is the primary objective across all modified files.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Tip

You can customize the high-level summary generated by CodeRabbit.

Configure the reviews.high_level_summary_instructions setting to provide custom instructions for generating the high-level summary.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
main/mimi_secrets.h.example (1)

20-24: ⚠️ Potential issue | 🟠 Major

Fix unterminated comment in LLM API section.
The /* ... comment is missing a closing */, which will comment out the rest of the file if users copy this template.

🔧 Proposed fix
-/* LLM API (anthropic, openai, openrouter, or nvidia_nim)
+/* LLM API (anthropic, openai, openrouter, or nvidia_nim) */
 `#define` MIMI_SECRET_API_KEY         ""
 `#define` MIMI_SECRET_MODEL           ""
 `#define` MIMI_SECRET_MODEL_PROVIDER  "anthropic"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@main/mimi_secrets.h.example` around lines 20 - 24, The LLM API section starts
a block comment with "/*" that is never closed, which will comment out
subsequent lines including the MIMI_SECRET_API_KEY, MIMI_SECRET_MODEL, and
MIMI_SECRET_MODEL_PROVIDER definitions; close the block comment by adding the
matching "*/" after the intended comment text (or remove the opening "/*" if the
lines are meant to be active) so the constants (MIMI_SECRET_API_KEY,
MIMI_SECRET_MODEL, MIMI_SECRET_MODEL_PROVIDER) are not accidentally commented
out.
main/llm/llm_proxy.c (1)

552-586: ⚠️ Potential issue | 🟠 Major

OpenRouter and NVIDIA NIM are OpenAI-compatible but use Anthropic request/response paths.

Request building, response parsing, and tool handling are guarded by provider_is_openai() only, so openrouter and nvidia_nim send Anthropic-shaped payloads and attempt Anthropic-style response parsing despite being OpenAI-compatible endpoints. This breaks both basic requests and tool use.

Introduce an OpenAI-compatibility predicate:

static bool provider_is_openai_compat(void)
{
    return provider_is_openai() || provider_is_openrouter() || provider_is_nvidia_nim();
}

Then replace provider_is_openai() with provider_is_openai_compat() in all request/response handling sections (lines 552–586, 638–642, 681–712, 757–809) for message conversion, token limits, response extraction, and tool call parsing.

Also applies to: 638-642, 681-712, 757-809

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@main/llm/llm_proxy.c` around lines 552 - 586, Add a new predicate
provider_is_openai_compat() that returns true when provider_is_openai() or
provider_is_openrouter() or provider_is_nvidia_nim() is true, then replace calls
to provider_is_openai() with provider_is_openai_compat() in all request/response
handling paths (the message conversion/token limit/response extraction/tool
parsing blocks that include convert_messages_openai, cJSON building for
"messages"/"system", and token keys like "max_tokens"/"max_completion_tokens")
so OpenRouter and NVIDIA NIM use the OpenAI-shaped payloads and parsing; update
the checks in the blocks currently guarded by provider_is_openai() (around the
message-building section and the other sections noted) to use
provider_is_openai_compat() instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/ARCHITECTURE.md`:
- Around line 57-63: Update the system overview diagram in ARCHITECTURE.md to
include NVIDIA NIM as an LLM provider: add "NVIDIA NIM" to the top provider list
line alongside "Anthropic/OpenAI/OpenRouter API (HTTPS)" and "Brave Search API
(HTTPS)" and ensure the ASCII diagram box or label for "LLM Provider (API
Gateway)" reflects NVIDIA NIM as one of the providers so the visual and text
lists remain consistent.

In `@main/llm/llm_proxy.c`:
- Around line 170-175: llm_api_path() returns the wrong proxy path for
OpenRouter (missing the /api/ prefix) causing proxy requests built in
llm_http_via_proxy() to 404; update llm_api_path() so that when
provider_is_openrouter() is true it returns "/api/v1/chat/completions" (keep
existing "/v1/chat/completions" for OpenAI and NVIDIA Nim and "/v1/messages" as
the default) so proxy-built paths match the full URL returned by llm_api_url().

In `@README.md`:
- Around line 34-35: Update the README text that currently says "NVIDIA NIM
(completely free)" to avoid the inaccurate claim: replace that phrase with a
conditional description such as "NVIDIA NIM (free for development/prototyping
for NVIDIA Developer Program members; production requires NVIDIA AI Enterprise
licensing)" and add a link to NVIDIA's official terms or developer program page;
locate the exact string "NVIDIA NIM (completely free)" in the README and update
it accordingly.

---

Outside diff comments:
In `@main/llm/llm_proxy.c`:
- Around line 552-586: Add a new predicate provider_is_openai_compat() that
returns true when provider_is_openai() or provider_is_openrouter() or
provider_is_nvidia_nim() is true, then replace calls to provider_is_openai()
with provider_is_openai_compat() in all request/response handling paths (the
message conversion/token limit/response extraction/tool parsing blocks that
include convert_messages_openai, cJSON building for "messages"/"system", and
token keys like "max_tokens"/"max_completion_tokens") so OpenRouter and NVIDIA
NIM use the OpenAI-shaped payloads and parsing; update the checks in the blocks
currently guarded by provider_is_openai() (around the message-building section
and the other sections noted) to use provider_is_openai_compat() instead.

In `@main/mimi_secrets.h.example`:
- Around line 20-24: The LLM API section starts a block comment with "/*" that
is never closed, which will comment out subsequent lines including the
MIMI_SECRET_API_KEY, MIMI_SECRET_MODEL, and MIMI_SECRET_MODEL_PROVIDER
definitions; close the block comment by adding the matching "*/" after the
intended comment text (or remove the opening "/*" if the lines are meant to be
active) so the constants (MIMI_SECRET_API_KEY, MIMI_SECRET_MODEL,
MIMI_SECRET_MODEL_PROVIDER) are not accidentally commented out.

---

Duplicate comments:
In `@docs/ARCHITECTURE.md`:
- Around line 282-289: The documentation makes a potentially incorrect claim
that "NVIDIA NIM" is "completely free" in the LLM Provider Integration section;
update the NVIDIA NIM bullet (the string "NVIDIA NIM" / enum key `nvidia_nim`)
to remove or qualify the "completely free" phrase and replace it with accurate,
verifiable wording (e.g., state any free tier limits, trial details, or link to
NVIDIA NIM pricing), ensuring the provider list remains accurate and
non-duplicative.

@IRONICBo IRONICBo self-requested a review February 19, 2026 15:58
@IRONICBo
Copy link
Copy Markdown
Member

Thanks for your contribution! I will take a look about it.

@MauroDruwel
Copy link
Copy Markdown
Author

I changed nvidia_nim to nvidia to avoid issues

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@README.md`:
- Line 43: Replace the misleading "NVIDIA NIM (free)" phrasing in README.md (the
string "NVIDIA NIM (free)") with wording that mirrors NVIDIA's official terms:
state that API access is available free for development/prototyping via the
NVIDIA Developer Program but production use requires NVIDIA AI Enterprise
licensing; update all occurrences (including the other spots noted around lines
181-184) to the same clarified text so the README consistently reflects the
Developer Program vs production licensing distinction.

@MauroDruwel
Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 20, 2026

✅ 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.

Copy link
Copy Markdown
Member

@IRONICBo IRONICBo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-structured addition of two OpenAI-compatible providers. The provider_is_openai_compat() abstraction is clean. Issues:

  1. Merge conflicts — needs rebase onto current main.
  2. README and ARCHITECTURE.md contain verbose NVIDIA licensing disclaimers repeated 4+ times. Mention it once in the setup section and link to NVIDIA's terms page instead.
  3. Minor: llm_api_host() returns hardcoded strings per provider. If custom URL support lands (PR #40), these will diverge. Acceptable for now but worth a TODO comment.

feat: add TODO in llm_proxy.c for deriving host from configured API URL
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/ARCHITECTURE.md (1)

79-85: ⚠️ Potential issue | 🟡 Minor

Update provider-specific stop/tool-call semantics in Data Flow.

Line 79 generalizes to “LLM provider API”, but Lines 81–85 still describe only Anthropic (stop_reason). OpenAI-compatible providers use finish_reason (e.g., tool_calls).

📝 Suggested wording update
-      iii. If stop_reason == "tool_use":
+      iii. If Anthropic stop_reason == "tool_use" or OpenAI-compatible finish_reason == "tool_calls":
...
-      iv.  If stop_reason == "end_turn": break with final text
+      iv.  If completion indicates done (Anthropic: "end_turn"; OpenAI-compatible: non-tool-call finish_reason): break with final text
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/ARCHITECTURE.md` around lines 79 - 85, The Data Flow wording currently
assumes Anthropic-style stop semantics (stop_reason with values like "tool_use"
and "end_turn"); update the description to explicitly cover provider-specific
fields by mentioning both Anthropic's stop_reason and OpenAI-style finish_reason
(e.g., "tool_calls"/"stop" semantics), and suggest treating them equivalently
(map finish_reason=="tool_calls" to stop_reason=="tool_use") so the flow reads:
call LLM provider API → parse JSON response for provider-specific stop/finish
field (stop_reason or finish_reason) and tool blocks → if provider indicates a
tool call (stop_reason == "tool_use" or finish_reason == "tool_calls") execute
tools, append results to messages and continue loop → if provider indicates
final turn (stop_reason == "end_turn" or finish_reason == "stop") break with
final text.
🧹 Nitpick comments (1)
main/llm/llm_proxy.c (1)

248-249: Use provider_is_openai_compat() consistently in HTTP header branches.

Lines 248 and 276 duplicate the compatibility condition manually. Reuse the helper to keep provider logic centralized.

♻️ Suggested refactor
-    if (provider_is_openai() || provider_is_openrouter() || provider_is_nvidia()) {
+    if (provider_is_openai_compat()) {
...
-    if (provider_is_openai() || provider_is_openrouter() || provider_is_nvidia()) {
+    if (provider_is_openai_compat()) {

Also applies to: 276-277

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@main/llm/llm_proxy.c` around lines 248 - 249, The code duplicates the
provider compatibility check by manually combining provider_is_openai(),
provider_is_openrouter(), and provider_is_nvidia() where HTTP headers are set;
replace those compound conditions with a call to the central helper
provider_is_openai_compat() (and remove the old combined checks) so both header
branches (the block checking s_api_key and the later block around lines 276-277)
consistently use provider_is_openai_compat() to decide OpenAI-compatible header
behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/ARCHITECTURE.md`:
- Around line 236-237: The docs claim there is "no runtime configuration" but
the code actually supports runtime overrides via CLI and NVS; update the
ARCHITECTURE.md text around the configuration section so it no longer asserts
"no runtime configuration" and instead documents that environment variables like
MIMI_SECRET_API_KEY and MIMI_SECRET_MODEL can be overridden at runtime using the
CLI and NVS (include brief notes on precedence and default model value
claude-opus-4-6); ensure the table entries for MIMI_SECRET_API_KEY and
MIMI_SECRET_MODEL remain but add a sentence referencing the supported runtime
overrides (CLI/NVS) and the override precedence.

In `@main/llm/llm_proxy.c`:
- Around line 147-150: The provider_is_nvidia function only checks for "nvidia"
and breaks compatibility with legacy "nvidia_nim"; update provider_is_nvidia to
treat both "nvidia" and "nvidia_nim" as NVIDIA providers (e.g., check
strcmp(s_provider, "nvidia")==0 || strcmp(s_provider, "nvidia_nim")==0 or test a
prefix like strncmp(s_provider, "nvidia", 6)==0) so routing uses the NVIDIA
URL/headers when s_provider holds either value; modify the provider_is_nvidia
function accordingly.

---

Outside diff comments:
In `@docs/ARCHITECTURE.md`:
- Around line 79-85: The Data Flow wording currently assumes Anthropic-style
stop semantics (stop_reason with values like "tool_use" and "end_turn"); update
the description to explicitly cover provider-specific fields by mentioning both
Anthropic's stop_reason and OpenAI-style finish_reason (e.g.,
"tool_calls"/"stop" semantics), and suggest treating them equivalently (map
finish_reason=="tool_calls" to stop_reason=="tool_use") so the flow reads: call
LLM provider API → parse JSON response for provider-specific stop/finish field
(stop_reason or finish_reason) and tool blocks → if provider indicates a tool
call (stop_reason == "tool_use" or finish_reason == "tool_calls") execute tools,
append results to messages and continue loop → if provider indicates final turn
(stop_reason == "end_turn" or finish_reason == "stop") break with final text.

---

Nitpick comments:
In `@main/llm/llm_proxy.c`:
- Around line 248-249: The code duplicates the provider compatibility check by
manually combining provider_is_openai(), provider_is_openrouter(), and
provider_is_nvidia() where HTTP headers are set; replace those compound
conditions with a call to the central helper provider_is_openai_compat() (and
remove the old combined checks) so both header branches (the block checking
s_api_key and the later block around lines 276-277) consistently use
provider_is_openai_compat() to decide OpenAI-compatible header behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f0fdbe8a-2c2f-4b17-a507-a36c67a6ead8

📥 Commits

Reviewing files that changed from the base of the PR and between 9c9d720 and f57343b.

📒 Files selected for processing (4)
  • README.md
  • docs/ARCHITECTURE.md
  • main/cli/serial_cli.c
  • main/llm/llm_proxy.c
🚧 Files skipped from review as they are similar to previous changes (1)
  • main/cli/serial_cli.c

Comment on lines +236 to 237
| `MIMI_SECRET_API_KEY` | LLM API key (Anthropic, OpenAI, OpenRouter, or NVIDIA NIM) |
| `MIMI_SECRET_MODEL` | Model ID (default: claude-opus-4-6) |
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Configuration section still conflicts with runtime override behavior.

Around this section, Line 229 states there is no runtime configuration, but current behavior uses CLI + NVS overrides. This will mislead operators.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/ARCHITECTURE.md` around lines 236 - 237, The docs claim there is "no
runtime configuration" but the code actually supports runtime overrides via CLI
and NVS; update the ARCHITECTURE.md text around the configuration section so it
no longer asserts "no runtime configuration" and instead documents that
environment variables like MIMI_SECRET_API_KEY and MIMI_SECRET_MODEL can be
overridden at runtime using the CLI and NVS (include brief notes on precedence
and default model value claude-opus-4-6); ensure the table entries for
MIMI_SECRET_API_KEY and MIMI_SECRET_MODEL remain but add a sentence referencing
the supported runtime overrides (CLI/NVS) and the override precedence.

Comment on lines +147 to +150
static bool provider_is_nvidia(void)
{
return strcmp(s_provider, "nvidia") == 0;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Preserve backward compatibility for prior nvidia_nim provider values.

Line 149 only matches "nvidia". If NVS/build-time config still contains "nvidia_nim", routing falls back to Anthropic URL/headers and requests fail.

🔧 Suggested fix
 static bool provider_is_nvidia(void)
 {
-    return strcmp(s_provider, "nvidia") == 0;
+    return strcmp(s_provider, "nvidia") == 0 ||
+           strcmp(s_provider, "nvidia_nim") == 0; /* backward-compatible alias */
 }
📝 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
static bool provider_is_nvidia(void)
{
return strcmp(s_provider, "nvidia") == 0;
}
static bool provider_is_nvidia(void)
{
return strcmp(s_provider, "nvidia") == 0 ||
strcmp(s_provider, "nvidia_nim") == 0; /* backward-compatible alias */
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@main/llm/llm_proxy.c` around lines 147 - 150, The provider_is_nvidia function
only checks for "nvidia" and breaks compatibility with legacy "nvidia_nim";
update provider_is_nvidia to treat both "nvidia" and "nvidia_nim" as NVIDIA
providers (e.g., check strcmp(s_provider, "nvidia")==0 || strcmp(s_provider,
"nvidia_nim")==0 or test a prefix like strncmp(s_provider, "nvidia", 6)==0) so
routing uses the NVIDIA URL/headers when s_provider holds either value; modify
the provider_is_nvidia function accordingly.

…portal, SPIFFS fix

- Resolved conflict in main/llm/llm_proxy.c: removed dead llm_chat() backward-compat stub (not declared in header, not referenced); NVIDIA NIM support retained
- Resolved conflict in main/mimi_secrets.h.example: kept upstream Feishu Bot section plus fork's descriptive LLM provider comment
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
main/llm/llm_proxy.c (1)

195-198: ⚠️ Potential issue | 🟠 Major

Preserve backward compatibility for nvidia_nim provider value.

The function only matches "nvidia", but existing NVS configurations may contain "nvidia_nim" from before the rename. These configurations would silently fall back to Anthropic routing, causing authentication failures.

🔧 Proposed fix
 static bool provider_is_nvidia(void)
 {
-    return strcmp(s_provider, "nvidia") == 0;
+    return strcmp(s_provider, "nvidia") == 0 ||
+           strcmp(s_provider, "nvidia_nim") == 0;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@main/llm/llm_proxy.c` around lines 195 - 198, provider_is_nvidia currently
only returns true for s_provider == "nvidia", which breaks backward
compatibility for the legacy "nvidia_nim" value; update provider_is_nvidia to
treat both "nvidia" and "nvidia_nim" as NVIDIA providers (e.g., check
strcmp(s_provider, "nvidia") == 0 || strcmp(s_provider, "nvidia_nim") == 0) so
existing NVS configs continue to route correctly while preserving the s_provider
variable and function name provider_is_nvidia.
🧹 Nitpick comments (2)
main/llm/llm_proxy.c (2)

324-343: Same refactor opportunity applies here.

This inline provider check mirrors the one in llm_http_direct(). Consider using provider_is_openai_compat() for both locations.

♻️ Proposed refactor
-    if (provider_is_openai() || provider_is_openrouter() || provider_is_nvidia()) {
+    if (provider_is_openai_compat()) {
         hlen = snprintf(header, sizeof(header),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@main/llm/llm_proxy.c` around lines 324 - 343, The header-construction block
in llm_proxy.c duplicates the provider check used elsewhere; replace the
compound check (provider_is_openai() || provider_is_openrouter() ||
provider_is_nvidia()) with the single helper provider_is_openai_compat() to
centralize provider logic, keeping the two snprintf branches and the same
arguments (llm_api_path(), llm_api_host(), s_api_key, body_len or
MIMI_LLM_API_VERSION) intact; this mirrors the refactor already applied in
llm_http_direct() and prevents divergent provider logic.

296-305: Consider using provider_is_openai_compat() for consistency.

The inline check duplicates the logic in provider_is_openai_compat() which is used elsewhere in this file. Using the helper would improve maintainability if provider grouping changes.

♻️ Proposed refactor
-    if (provider_is_openai() || provider_is_openrouter() || provider_is_nvidia()) {
+    if (provider_is_openai_compat()) {
         if (s_api_key[0]) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@main/llm/llm_proxy.c` around lines 296 - 305, Replace the inline provider
check (provider_is_openai() || provider_is_openrouter() || provider_is_nvidia())
with the helper provider_is_openai_compat() to centralize provider grouping;
keep the existing behavior: when provider_is_openai_compat() is true and
s_api_key is non-empty set the "Authorization" header to "Bearer <s_api_key>"
(using the existing auth buffer logic referencing LLM_API_KEY_MAX_LEN and
esp_http_client_set_header), otherwise set "x-api-key" to s_api_key and
"anthropic-version" to MIMI_LLM_API_VERSION so the rest of the code
(esp_http_client_set_header, s_api_key, MIMI_LLM_API_VERSION) continues to work
as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@main/llm/llm_proxy.c`:
- Around line 195-198: provider_is_nvidia currently only returns true for
s_provider == "nvidia", which breaks backward compatibility for the legacy
"nvidia_nim" value; update provider_is_nvidia to treat both "nvidia" and
"nvidia_nim" as NVIDIA providers (e.g., check strcmp(s_provider, "nvidia") == 0
|| strcmp(s_provider, "nvidia_nim") == 0) so existing NVS configs continue to
route correctly while preserving the s_provider variable and function name
provider_is_nvidia.

---

Nitpick comments:
In `@main/llm/llm_proxy.c`:
- Around line 324-343: The header-construction block in llm_proxy.c duplicates
the provider check used elsewhere; replace the compound check
(provider_is_openai() || provider_is_openrouter() || provider_is_nvidia()) with
the single helper provider_is_openai_compat() to centralize provider logic,
keeping the two snprintf branches and the same arguments (llm_api_path(),
llm_api_host(), s_api_key, body_len or MIMI_LLM_API_VERSION) intact; this
mirrors the refactor already applied in llm_http_direct() and prevents divergent
provider logic.
- Around line 296-305: Replace the inline provider check (provider_is_openai()
|| provider_is_openrouter() || provider_is_nvidia()) with the helper
provider_is_openai_compat() to centralize provider grouping; keep the existing
behavior: when provider_is_openai_compat() is true and s_api_key is non-empty
set the "Authorization" header to "Bearer <s_api_key>" (using the existing auth
buffer logic referencing LLM_API_KEY_MAX_LEN and esp_http_client_set_header),
otherwise set "x-api-key" to s_api_key and "anthropic-version" to
MIMI_LLM_API_VERSION so the rest of the code (esp_http_client_set_header,
s_api_key, MIMI_LLM_API_VERSION) continues to work as before.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 64567f3d-2047-4f1a-9d47-f6f6a9e70cf6

📥 Commits

Reviewing files that changed from the base of the PR and between f57343b and 0699012.

📒 Files selected for processing (6)
  • README.md
  • main/cli/serial_cli.c
  • main/llm/llm_proxy.c
  • main/llm/llm_proxy.h
  • main/mimi_config.h
  • main/mimi_secrets.h.example
✅ Files skipped from review due to trivial changes (1)
  • main/llm/llm_proxy.h
🚧 Files skipped from review as they are similar to previous changes (2)
  • main/cli/serial_cli.c
  • main/mimi_secrets.h.example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants