C7: move LLM env/credentials to praisonai-code for standalone gate#2548
Conversation
Credential checks and endpoint resolution no longer import the wrapper; shims preserve praisonai.llm.* module identity and CI smoke verifies standalone import.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThe PR adds Changespraisonai_code LLM env/credentials migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Model listing and AutoGen config_list building no longer import the wrapper; shims preserve praisonai.llm.* module identity for backward compatibility.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request moves the LLM environment variable resolution and credential handling from the praisonai wrapper package to the standalone praisonai_code package, establishing backward-compatibility shims in the original paths. The review feedback highlights that the openrouter provider is missing from several credential and environment resolution helpers, which could prevent OpenRouter credentials from being correctly exported, checked, or resolved. Additionally, the reviewer suggests adding defensive None checks for import_wrapper_module in app.py to avoid potential AttributeError exceptions when running in standalone mode.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| env_mappings = { | ||
| "openai": "OPENAI_API_KEY", | ||
| "anthropic": "ANTHROPIC_API_KEY", | ||
| "google": "GOOGLE_API_KEY", | ||
| "gemini": "GEMINI_API_KEY", | ||
| "tavily": "TAVILY_API_KEY", | ||
| "groq": "GROQ_API_KEY", | ||
| "cohere": "COHERE_API_KEY", | ||
| } |
There was a problem hiding this comment.
The "openrouter" provider is missing from the env_mappings dictionary. Without this mapping, inject_credentials_into_env will not export stored OpenRouter credentials to the environment, causing subsequent calls to fail when they look for OPENROUTER_API_KEY.
| env_mappings = { | |
| "openai": "OPENAI_API_KEY", | |
| "anthropic": "ANTHROPIC_API_KEY", | |
| "google": "GOOGLE_API_KEY", | |
| "gemini": "GEMINI_API_KEY", | |
| "tavily": "TAVILY_API_KEY", | |
| "groq": "GROQ_API_KEY", | |
| "cohere": "COHERE_API_KEY", | |
| } | |
| env_mappings = { | |
| "openai": "OPENAI_API_KEY", | |
| "anthropic": "ANTHROPIC_API_KEY", | |
| "google": "GOOGLE_API_KEY", | |
| "gemini": "GEMINI_API_KEY", | |
| "tavily": "TAVILY_API_KEY", | |
| "groq": "GROQ_API_KEY", | |
| "cohere": "COHERE_API_KEY", | |
| "openrouter": "OPENROUTER_API_KEY", | |
| } |
| def _provider_key_vars_for_model(model: str) -> tuple[str, ...]: | ||
| """Map a model id to the environment variable(s) for its provider.""" | ||
| m = model.lower() | ||
| if m.startswith("anthropic/") or m.startswith("claude"): | ||
| return ("ANTHROPIC_API_KEY",) | ||
| if m.startswith("google/"): | ||
| return ("GOOGLE_API_KEY",) | ||
| if m.startswith("gemini/") or m.startswith("gemini"): | ||
| return ("GEMINI_API_KEY",) | ||
| if m.startswith("groq/"): | ||
| return ("GROQ_API_KEY",) | ||
| if m.startswith("cohere/"): | ||
| return ("COHERE_API_KEY",) | ||
| if m.startswith("ollama/"): | ||
| return ("OLLAMA_HOST",) | ||
| if ( | ||
| m.startswith("gpt") | ||
| or m.startswith("o1") | ||
| or m.startswith("o3") | ||
| or m.startswith("o4") | ||
| or m.startswith("openai/") | ||
| ): | ||
| return ("OPENAI_API_KEY",) | ||
| return () |
There was a problem hiding this comment.
The "openrouter/" model prefix is missing from _provider_key_vars_for_model. This causes is_configured to return False for explicit OpenRouter models even when OPENROUTER_API_KEY is set, blocking execution in non-interactive environments.
def _provider_key_vars_for_model(model: str) -> tuple[str, ...]:
"""Map a model id to the environment variable(s) for its provider."""
m = model.lower()
if m.startswith("anthropic/") or m.startswith("claude"):
return ("ANTHROPIC_API_KEY",)
if m.startswith("google/"):
return ("GOOGLE_API_KEY",)
if m.startswith("gemini/") or m.startswith("gemini"):
return ("GEMINI_API_KEY",)
if m.startswith("groq/"):
return ("GROQ_API_KEY",)
if m.startswith("cohere/"):
return ("COHERE_API_KEY",)
if m.startswith("ollama/"):
return ("OLLAMA_HOST",)
if m.startswith("openrouter/"):
return ("OPENROUTER_API_KEY",)
if (
m.startswith("gpt")
or m.startswith("o1")
or m.startswith("o3")
or m.startswith("o4")
or m.startswith("openai/")
):
return ("OPENAI_API_KEY",)
return ()| _VAR_TO_STORED_PROVIDERS = { | ||
| "OPENAI_API_KEY": ("openai",), | ||
| "ANTHROPIC_API_KEY": ("anthropic",), | ||
| "GEMINI_API_KEY": ("gemini", "google"), | ||
| "GOOGLE_API_KEY": ("google", "gemini"), | ||
| "GROQ_API_KEY": ("groq",), | ||
| "COHERE_API_KEY": ("cohere",), | ||
| "OLLAMA_HOST": ("ollama",), | ||
| } |
There was a problem hiding this comment.
The "OPENROUTER_API_KEY" mapping is missing from _VAR_TO_STORED_PROVIDERS. This prevents is_configured from checking stored credentials for OpenRouter models.
| _VAR_TO_STORED_PROVIDERS = { | |
| "OPENAI_API_KEY": ("openai",), | |
| "ANTHROPIC_API_KEY": ("anthropic",), | |
| "GEMINI_API_KEY": ("gemini", "google"), | |
| "GOOGLE_API_KEY": ("google", "gemini"), | |
| "GROQ_API_KEY": ("groq",), | |
| "COHERE_API_KEY": ("cohere",), | |
| "OLLAMA_HOST": ("ollama",), | |
| } | |
| _VAR_TO_STORED_PROVIDERS = { | |
| "OPENAI_API_KEY": ("openai",), | |
| "ANTHROPIC_API_KEY": ("anthropic",), | |
| "GEMINI_API_KEY": ("gemini", "google"), | |
| "GOOGLE_API_KEY": ("google", "gemini"), | |
| "GROQ_API_KEY": ("groq",), | |
| "COHERE_API_KEY": ("cohere",), | |
| "OLLAMA_HOST": ("ollama",), | |
| "OPENROUTER_API_KEY": ("openrouter",), | |
| } |
| known_keys = ( | ||
| "OPENAI_API_KEY", "ANTHROPIC_API_KEY", "GOOGLE_API_KEY", | ||
| "GEMINI_API_KEY", "GROQ_API_KEY", "COHERE_API_KEY", | ||
| "OLLAMA_HOST", | ||
| ) |
There was a problem hiding this comment.
The "OPENROUTER_API_KEY" environment variable is missing from known_keys in is_configured. This causes the general credential check to fail when only OPENROUTER_API_KEY is configured.
| known_keys = ( | |
| "OPENAI_API_KEY", "ANTHROPIC_API_KEY", "GOOGLE_API_KEY", | |
| "GEMINI_API_KEY", "GROQ_API_KEY", "COHERE_API_KEY", | |
| "OLLAMA_HOST", | |
| ) | |
| known_keys = ( | |
| "OPENAI_API_KEY", "ANTHROPIC_API_KEY", "GOOGLE_API_KEY", | |
| "GEMINI_API_KEY", "GROQ_API_KEY", "COHERE_API_KEY", | |
| "OLLAMA_HOST", "OPENROUTER_API_KEY", | |
| ) |
| fallback_base = None | ||
| if not api_key and fallback_lookup: | ||
| try: | ||
| for provider in ["openai", "anthropic", "google", "gemini", "groq", "cohere"]: |
There was a problem hiding this comment.
The "openrouter" provider is missing from the fallback lookup list. This prevents resolve_llm_endpoint from retrieving stored OpenRouter credentials when the environment variable is not set.
| for provider in ["openai", "anthropic", "google", "gemini", "groq", "cohere"]: | |
| for provider in ["openai", "anthropic", "google", "gemini", "groq", "cohere", "openrouter"]: |
| def _setup_langfuse_observability(*, verbose: bool = False) -> None: | ||
| """Set up Langfuse observability by wiring TraceSink to action emitter.""" | ||
| try: | ||
| from praisonai.observability.langfuse import LangfuseSink | ||
| from praisonai_code._wrapper_bridge import import_wrapper_module | ||
| langfuse = import_wrapper_module("praisonai.observability.langfuse") | ||
| LangfuseSink = langfuse.LangfuseSink |
There was a problem hiding this comment.
In standalone mode, import_wrapper_module may return None if the wrapper is not installed. Accessing langfuse.LangfuseSink directly without a None check will raise an AttributeError. Adding a defensive guard prevents unnecessary exceptions and warnings when running in standalone mode.
| def _setup_langfuse_observability(*, verbose: bool = False) -> None: | |
| """Set up Langfuse observability by wiring TraceSink to action emitter.""" | |
| try: | |
| from praisonai.observability.langfuse import LangfuseSink | |
| from praisonai_code._wrapper_bridge import import_wrapper_module | |
| langfuse = import_wrapper_module("praisonai.observability.langfuse") | |
| LangfuseSink = langfuse.LangfuseSink | |
| def _setup_langfuse_observability(*, verbose: bool = False) -> None: | |
| """Set up Langfuse observability by wiring TraceSink to action emitter.""" | |
| try: | |
| from praisonai_code._wrapper_bridge import import_wrapper_module | |
| langfuse = import_wrapper_module("praisonai.observability.langfuse") | |
| if langfuse is None: | |
| return | |
| LangfuseSink = langfuse.LangfuseSink |
| from praisonai_code._wrapper_bridge import import_wrapper_module | ||
| langextract_mod = import_wrapper_module("praisonai.observability.langextract") | ||
| LangextractSink = langextract_mod.LangextractSink | ||
| LangextractSinkConfig = langextract_mod.LangextractSinkConfig |
There was a problem hiding this comment.
In standalone mode, import_wrapper_module may return None if the wrapper is not installed. Accessing langextract_mod.LangextractSink directly without a None check will raise an AttributeError. Adding a defensive guard prevents unnecessary exceptions and warnings when running in standalone mode.
| from praisonai_code._wrapper_bridge import import_wrapper_module | |
| langextract_mod = import_wrapper_module("praisonai.observability.langextract") | |
| LangextractSink = langextract_mod.LangextractSink | |
| LangextractSinkConfig = langextract_mod.LangextractSinkConfig | |
| from praisonai_code._wrapper_bridge import import_wrapper_module | |
| langextract_mod = import_wrapper_module("praisonai.observability.langextract") | |
| if langextract_mod is None: | |
| return | |
| LangextractSink = langextract_mod.LangextractSink | |
| LangextractSinkConfig = langextract_mod.LangextractSinkConfig |
Greptile SummaryThis PR moves the LLM helper layer into
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (7): Last reviewed commit: "fix(c7): recover stored base_url for exp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/praisonai-code/praisonai_code/cli/app.py (1)
696-727: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExport stored credentials before starting the TUI.
The new
is_configured()can returnTruefromCredentialStore, but this path never callsinject_credentials_into_env(). A user configured viapraisonai setupcan pass the gate and then start the TUI without the key in process env;cli/main.pyalready injects before checking.🐛 Proposed fix
- from praisonai_code.llm.credentials import is_configured + from praisonai_code.llm.credentials import inject_credentials_into_env, is_configured import sys + inject_credentials_into_env() if not is_configured(): # Check for any configured credentials @@ if exit_code != 0: typer.echo("Setup failed. Exiting.", err=True) raise typer.Exit(exit_code) # Re-check credentials after setup + inject_credentials_into_env() if not is_configured():🤖 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/praisonai-code/praisonai_code/cli/app.py` around lines 696 - 727, The credential check path in the CLI currently allows `is_configured()` to pass without exporting stored credentials into the process environment. Update the flow around the existing `is_configured()` check in `app.py` to call `inject_credentials_into_env()` before starting the TUI, matching the behavior already used in `cli/main.py`; keep the existing setup-wizard fallback intact and ensure the env injection happens after successful setup and before any TUI launch.src/praisonai-code/praisonai_code/cli/commands/run.py (1)
531-563: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInject stored credentials before the early run gate.
is_configured(model)can now returnTruefrom stored credentials, butrun_main()does not export them before continuing. That letspraisonai setupusers pass the gate and still fail later when the LLM call reads env vars.🐛 Proposed fix
- from praisonai_code.llm.credentials import is_configured + from praisonai_code.llm.credentials import inject_credentials_into_env, is_configured import sys # Check if credentials are configured (use model if provided, else check general) + inject_credentials_into_env() if not is_configured(model): @@ output.print_success("Setup complete! Continuing with your run...") # Re-check after setup + inject_credentials_into_env() if not is_configured(model):🤖 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/praisonai-code/praisonai_code/cli/commands/run.py` around lines 531 - 563, `run_main()` is checking `is_configured(model)` before exporting any stored credentials, so the command can pass the gate and then fail later when the LLM client reads environment variables. Update the credential-check flow in `run_main()` to load/export stored credentials before the early exit path, using the existing `is_configured` and setup-related logic so later model calls can see the keys. Keep the fix localized to the run command’s preflight section and ensure the non-interactive and interactive branches both operate on the same exported credential state.
🤖 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 `@src/praisonai-code/praisonai_code/llm/__init__.py`:
- Around line 5-12: The package-level exports in llm/__init__.py are listed in
__all__ but the symbols are not actually bound in the module namespace, so
imports like from praisonai_code.llm import LLMEndpoint and wildcard imports
will fail. Update the llm package initializer to import and re-export the named
symbols from their defining module, keeping __all__ in sync with the actual
bindings for LLMEndpoint, default_model_for_available_provider,
resolve_llm_endpoint, inject_credentials_into_env, is_configured, and
resolve_llm_endpoint_with_credentials.
In `@src/praisonai-code/praisonai_code/llm/credentials.py`:
- Around line 61-69: OpenRouter is missing from the credential lookup in
credentials.py, so openrouter models can fail the API-key check even when
OPENROUTER_API_KEY is set. Update the env_mappings used by the credential gate
to include openrouter -> OPENROUTER_API_KEY, ensure the OpenRouter model path is
recognized by the same lookup logic that handles openai/anthropic/google, and
add OPENROUTER_API_KEY to known_keys so it is treated as a valid configured
credential.
In `@src/praisonai-code/praisonai_code/llm/env.py`:
- Around line 42-51: Add OpenRouter to the provider-aware default model mapping
in env.py. Update the _PROVIDER_DEFAULTS tuple used by the default-selection
logic so OPENROUTER_API_KEY is recognized alongside the other provider keys, and
give it an OpenRouter-compatible fallback model. This should be done in the same
place that defines the provider defaults so the existing lookup path can choose
the correct model when only an OpenRouter key is present.
- Around line 100-105: Align `_provider_from_model` with `is_configured` so bare
`claude...` and `gemini...` model IDs resolve to the Anthropic/Gemini provider
metadata instead of falling through to OpenAI defaults. Update the prefix
matching logic in `_provider_from_model` to recognize the same model patterns
handled by the credential gate, and ensure the returned key variable and base
URL match the provider inferred by `is_configured()` and any related provider
map entries.
- Around line 146-164: Scope the stored-credential fallback in env.py so it only
uses the provider implied by the resolved model when MODEL_NAME or config is
explicit. Update the fallback_lookup loop to prefer the selected provider from
the parsed model (and keep the broader scan only for inferred defaults), and
ensure api_key, fallback_model, and fallback_base are not mixed across providers
in the env resolution logic.
---
Outside diff comments:
In `@src/praisonai-code/praisonai_code/cli/app.py`:
- Around line 696-727: The credential check path in the CLI currently allows
`is_configured()` to pass without exporting stored credentials into the process
environment. Update the flow around the existing `is_configured()` check in
`app.py` to call `inject_credentials_into_env()` before starting the TUI,
matching the behavior already used in `cli/main.py`; keep the existing
setup-wizard fallback intact and ensure the env injection happens after
successful setup and before any TUI launch.
In `@src/praisonai-code/praisonai_code/cli/commands/run.py`:
- Around line 531-563: `run_main()` is checking `is_configured(model)` before
exporting any stored credentials, so the command can pass the gate and then fail
later when the LLM client reads environment variables. Update the
credential-check flow in `run_main()` to load/export stored credentials before
the early exit path, using the existing `is_configured` and setup-related logic
so later model calls can see the keys. Keep the fix localized to the run
command’s preflight section and ensure the non-interactive and interactive
branches both operate on the same exported credential state.
🪄 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
Run ID: 2c645514-8b7c-4515-8ac2-0f9bbf93e4f0
📒 Files selected for processing (14)
.github/workflows/test-optimized.ymlsrc/praisonai-agents/AGENTS.mdsrc/praisonai-code/README.mdsrc/praisonai-code/praisonai_code/cli/app.pysrc/praisonai-code/praisonai_code/cli/commands/init.pysrc/praisonai-code/praisonai_code/cli/commands/run.pysrc/praisonai-code/praisonai_code/cli/configuration/model_resolver.pysrc/praisonai-code/praisonai_code/cli/main.pysrc/praisonai-code/praisonai_code/llm/__init__.pysrc/praisonai-code/praisonai_code/llm/credentials.pysrc/praisonai-code/praisonai_code/llm/env.pysrc/praisonai/praisonai/llm/credentials.pysrc/praisonai/praisonai/llm/env.pysrc/praisonai/tests/unit/test_c5_backward_compat.py
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/blocked:ci
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2548 ✅Reviewed against AGENTS.md, incorporated findings from Gemini, CodeRabbit, Greptile, Qodo & Copilot. All valid issues fixed and pushed to Verification: 94/94 tests pass (
Phase 1 — Architecture (AGENTS.md)
Phase 2 — Fixes Applied
Verified live: Skipped (with reason)
Phase 3 — Verdict✅ Approve. All blocking (P1) reviewer findings resolved, backward compat preserved, 94/94 tests green. The OpenRouter gate gap was pre-existing ( Job run • branch |
…lback - Bind __all__ names in praisonai_code.llm/__init__.py so direct/star imports work - Add OpenRouter to credential gate (env mapping, model-key vars, stored-provider map, known_keys) and to env.py provider defaults + fallback scan - Align _provider_from_model with is_configured for bare claude/gemini model ids - Scope stored-credential fallback to the resolved provider for explicit/config models; only scan all providers for inferred defaults - Inject stored credentials before is_configured gate in app.py/run.py - Guard None from import_wrapper_module in observability setup (standalone mode) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2548 ✅Verified all reviewer findings (Gemini, CodeRabbit, Greptile, Qodo, Copilot) against the current branch state. All valid P1/major issues are already resolved in commit
Phase 1 — Architecture (AGENTS.md)
Phase 2 — Reviewer Findings: all confirmed FIXED
Verification (live, standalone — no
|
…__all__ - catalogue.py: use 'is not None' so zero-cost (free/local) models keep 0.0 cost instead of falling back to None (Gemini) - catalogue.py: validate_model now does a single describe_model lookup instead of is_valid_model + describe_model double pass (Gemini) - llm/__init__.py: bind all names advertised in __all__ so 'from praisonai_code.llm import *' and named imports resolve (Greptile) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
C7: move LLM catalogue and config to praisonai-code
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2548 ✅
Pushed Phase 1 — Architecture (AGENTS.md)
Phase 2 — Fix Applied This Round
Fix: for inferred (zero-config) default models only, consult the stored credential of the resolved provider to recover its saved
Behavioral verification (live): Confirmed Already-Fixed (prior commit
|
When inject_credentials_into_env() exports a stored provider key (e.g. OPENROUTER_API_KEY) into the environment, resolve_llm_endpoint saw the key already set and skipped the credential-store fallback, so a stored credential's custom model/base_url were dropped in favour of the hard-coded provider default. For inferred (zero-config) default models only, consult the stored credential of the resolved provider to recover its saved model/base_url. Explicit MODEL_NAME/config models and env base_url still take precedence, so no cross-provider mixing occurs. Addresses Greptile finding at credentials.py:88. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2548 ✅
Pushed Phase 1 — Architecture (AGENTS.md)
Phase 2 — Fix Applied This Round
Guardrails (verified live): Env vars ( Confirmed Already-Fixed (prior commits
|
Route build_config_list through the credential-store aware resolver so a stored credential's own model/base_url (e.g. a saved OpenRouter endpoint) is used after inject_credentials_into_env exports only the API key. Falls back to the env-only resolver if the credential bridge is unavailable; environment variables still take precedence inside the resolver. Addresses Greptile P1 (config.py:12 — stored endpoint dropped). Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2548 ✅
Pushed Phase 1 — Architecture (AGENTS.md)
Phase 2 — Fix Applied This Round
Reviewer Findings — all confirmed FIXED on current branch
Verification (live, standalone — no
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/praisonai-code/praisonai_code/llm/catalogue.py (1)
265-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep graceful fallback observable.
These paths silently collapse corrupted cache, permission, and
litellmmetadata failures into fallback mode. Add debug logging while preserving the non-fatal behavior.Proposed fix
import os import json import time import difflib +import logging from pathlib import Path @@ from dataclasses import dataclass, asdict +logger = logging.getLogger(__name__) + @@ - except Exception: + except Exception: # noqa: BLE001 - optional integration should not break fallback # Error loading from litellm, fall back + logger.debug("Failed to load model metadata from litellm", exc_info=True) return None @@ - except Exception: + except (OSError, json.JSONDecodeError, TypeError, ValueError, AttributeError): + logger.debug("Ignoring invalid model catalogue cache at %s", self.cache_file, exc_info=True) return None @@ - except Exception: + except (OSError, TypeError, ValueError): # Ignore cache write errors - pass + logger.debug("Unable to write model catalogue cache at %s", self.cache_file, exc_info=True)Also applies to: 282-316
🤖 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/praisonai-code/praisonai_code/llm/catalogue.py` around lines 265 - 270, The fallback paths in the litellm catalogue loading logic are swallowing errors silently, so add debug-level logging before returning None in the exception handlers. Update the ImportError and broad Exception branches in the catalogue load flow, including the related fallback blocks around the same litellm metadata/cache code path, to record the exception/context while keeping the behavior non-fatal. Use the existing catalogue-loading entrypoints and helper methods in praisonai_code.llm.catalogue so the graceful fallback remains observable without changing return values.Source: Linters/SAST tools
🤖 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 `@src/praisonai-code/praisonai_code/llm/catalogue.py`:
- Around line 68-87: The o1 and o1-mini fallback ModelInfo entries currently
claim “No streaming” in notes but still inherit streaming support, so update
those specific ModelInfo definitions in catalogue.py to explicitly set
supports_streaming=False alongside supports_tools=False. Keep the change limited
to the o1 fallback entries so models describe reflects the actual capabilities
shown by the notes.
- Around line 373-459: The model validation path in
validate_model/describe_model should normalize provider-qualified IDs before
treating them as unknown, since the fallback catalogue may store the same model
as a bare ID. Update the lookup flow to strip or map prefixes like anthropic/,
groq/, and ollama/ when matching against the fallback data, while keeping the
existing exact and case-insensitive checks intact. Use _load_from_litellm and
the fallback catalogue matching logic as the reference points, and preserve
backward compatibility by accepting both prefixed and bare forms rather than
changing the public API.
In `@src/praisonai-code/praisonai_code/llm/config.py`:
- Around line 10-12: build_config_list() is using the endpoint resolver
directly, which can create a config entry with api_key=None even after
credentials have been accepted. Update the logic in build_config_list() to call
resolve_llm_endpoint_with_credentials() instead of resolve_llm_endpoint(), and
keep the rest of the config assembly unchanged so the CLI runtime path always
uses the credential-aware fallback.
---
Nitpick comments:
In `@src/praisonai-code/praisonai_code/llm/catalogue.py`:
- Around line 265-270: The fallback paths in the litellm catalogue loading logic
are swallowing errors silently, so add debug-level logging before returning None
in the exception handlers. Update the ImportError and broad Exception branches
in the catalogue load flow, including the related fallback blocks around the
same litellm metadata/cache code path, to record the exception/context while
keeping the behavior non-fatal. Use the existing catalogue-loading entrypoints
and helper methods in praisonai_code.llm.catalogue so the graceful fallback
remains observable without changing return values.
🪄 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
Run ID: 0fbaf970-6661-4034-93dc-75175d6f6af1
📒 Files selected for processing (11)
.github/workflows/test-optimized.ymlsrc/praisonai-code/README.mdsrc/praisonai-code/praisonai_code/cli/commands/models.pysrc/praisonai-code/praisonai_code/cli/main.pysrc/praisonai-code/praisonai_code/llm/__init__.pysrc/praisonai-code/praisonai_code/llm/catalogue.pysrc/praisonai-code/praisonai_code/llm/config.pysrc/praisonai-code/praisonai_code/llm/env.pysrc/praisonai/praisonai/llm/catalogue.pysrc/praisonai/praisonai/llm/config.pysrc/praisonai/tests/unit/test_c5_backward_compat.py
✅ Files skipped from review due to trivial changes (2)
- src/praisonai/praisonai/llm/config.py
- src/praisonai-code/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/praisonai-code/praisonai_code/cli/main.py
- .github/workflows/test-optimized.yml
- src/praisonai-code/praisonai_code/llm/env.py
Remove duplicate re-imports in praisonai_code.llm.__init__ and sort __all__ (RUF022). No behavioral change; all exports remain bound. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2548 ✅
Pushed Phase 1 — Architecture (AGENTS.md)
Phase 2 — Fixes Applied This Round (
|
…l ids - Set supports_streaming=False on o1/o1-mini fallback entries so `models describe` reflects their "No streaming" notes (CodeRabbit) - describe_model now matches provider-qualified ids (e.g. groq/llama...) against bare fallback ids when litellm is unavailable (CodeRabbit) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2548 ✅
Pushed Phase 1 — Architecture (AGENTS.md)
Phase 2 — Fix Applied This Round
Previously the stored-endpoint recovery branch was gated on Fix: the recovery branch now runs whenever the key is present and a
Behavioral verification (live): Confirmed Already-Fixed (prior commits
|
… injection For explicit/config models, inject_credentials_into_env() only exports the API key (not the stored base_url) for non-OpenAI providers, and the resolver previously skipped the credential-store fallback entirely once the key was in env. A stored OpenRouter/Groq/Gemini credential with a custom base_url could pass the gate but send requests to the provider default URL. Now recover the stored base_url (never overriding the user-requested model) for explicit models, scoped to the resolved provider; env/config base_url still wins. Inferred defaults keep adopting the stored model as before. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2548 ✅
No new code changes required this round — every valid reviewer finding is already resolved on the current branch ( Phase 1 — Architecture (AGENTS.md)
Phase 2 — Reviewer Findings: all confirmed FIXED
Verification (live, standalone — no
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/praisonai-code/praisonai_code/llm/config.py (1)
17-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the fallback to
ImportErrorinstead of blindException.Catching
Exceptionhere doesn't just guard against the credentials module being unavailable — it also silently swallows any runtime error raised insideresolve_llm_endpoint_with_credentials()(e.g. a bug in credential lookup or catalogue validation), silently downgrading to the env-only resolver and potentially reintroducing anapi_key=Noneentry with no visible error. Since the intent is clearly "fall back if the credentials bridge module isn't importable," scope the catch accordingly.♻️ Proposed fix
try: from praisonai_code.llm.credentials import ( resolve_llm_endpoint_with_credentials, ) ep = resolve_llm_endpoint_with_credentials() - except Exception: + except ImportError: from praisonai_code.llm.env import resolve_llm_endpoint ep = resolve_llm_endpoint()🤖 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/praisonai-code/praisonai_code/llm/config.py` around lines 17 - 26, The fallback in config.py is too broad: the current try/except around resolve_llm_endpoint_with_credentials() catches all Exception and can hide real runtime failures. Narrow the exception handling to ImportError only, so the fallback to resolve_llm_endpoint() in the config module is used only when the credentials bridge cannot be imported. Keep the logic centered on resolve_llm_endpoint_with_credentials and resolve_llm_endpoint, and avoid swallowing other errors.Source: Linters/SAST tools
🤖 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 `@src/praisonai-code/praisonai_code/llm/env.py`:
- Around line 207-223: The fallback metadata lookup in env.py is reusing stored
model/base_url data without verifying it belongs to the same API key. Update the
fallback_lookup/provider handling in this branch so `cred["model"]` and
`cred["base_url"]` are only adopted when the stored credential key matches the
active env/config API key, and otherwise skip that stored metadata. Keep the
change localized around the fallback credential selection logic and any helper
that compares credential identity.
- Around line 226-227: The metadata recovery path in the environment handling
currently swallows all exceptions in the credential-store lookup, which hides
diagnosable failures. Update the exception handler in the relevant
metadata-recovery logic in env.py to keep the best-effort behavior but emit a
debug-level log with the caught exception instead of using a bare pass. Use the
surrounding env/credential-store recovery flow and any existing logger in this
module to preserve context and make failures traceable.
---
Nitpick comments:
In `@src/praisonai-code/praisonai_code/llm/config.py`:
- Around line 17-26: The fallback in config.py is too broad: the current
try/except around resolve_llm_endpoint_with_credentials() catches all Exception
and can hide real runtime failures. Narrow the exception handling to ImportError
only, so the fallback to resolve_llm_endpoint() in the config module is used
only when the credentials bridge cannot be imported. Keep the logic centered on
resolve_llm_endpoint_with_credentials and resolve_llm_endpoint, and avoid
swallowing other errors.
🪄 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
Run ID: f90c8806-f2d3-43ca-8f8f-ebcc015a12e9
📒 Files selected for processing (4)
src/praisonai-code/praisonai_code/llm/__init__.pysrc/praisonai-code/praisonai_code/llm/catalogue.pysrc/praisonai-code/praisonai_code/llm/config.pysrc/praisonai-code/praisonai_code/llm/env.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/praisonai-code/praisonai_code/llm/catalogue.py
| cred = fallback_lookup(provider) | ||
| if not cred: | ||
| continue | ||
| cred_model = cred.get("model") | ||
| if ( | ||
| not explicit_or_config_model | ||
| and cred_model | ||
| and not (env_base or config_base) | ||
| ): | ||
| if validate_model and catalogue: | ||
| try: | ||
| cred_model = catalogue.validate_model(cred_model) | ||
| except ValueError: | ||
| cred_model = None | ||
| fallback_model = cred_model or fallback_model | ||
| if not env_base and not config_base and cred.get("base_url"): | ||
| fallback_base = cred["base_url"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Only recover stored metadata for the same API key.
This branch can pair a manually supplied env API key with stale stored model/base_url metadata for the same provider. Require the stored credential key to match before adopting its endpoint metadata, otherwise an env key can be sent to an unintended custom base URL.
Proposed fix
cred = fallback_lookup(provider)
if not cred:
continue
+ if cred.get("api_key") != api_key:
+ continue
cred_model = cred.get("model")📝 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.
| cred = fallback_lookup(provider) | |
| if not cred: | |
| continue | |
| cred_model = cred.get("model") | |
| if ( | |
| not explicit_or_config_model | |
| and cred_model | |
| and not (env_base or config_base) | |
| ): | |
| if validate_model and catalogue: | |
| try: | |
| cred_model = catalogue.validate_model(cred_model) | |
| except ValueError: | |
| cred_model = None | |
| fallback_model = cred_model or fallback_model | |
| if not env_base and not config_base and cred.get("base_url"): | |
| fallback_base = cred["base_url"] | |
| cred = fallback_lookup(provider) | |
| if not cred: | |
| continue | |
| if cred.get("api_key") != api_key: | |
| continue | |
| cred_model = cred.get("model") | |
| if ( | |
| not explicit_or_config_model | |
| and cred_model | |
| and not (env_base or config_base) | |
| ): | |
| if validate_model and catalogue: | |
| try: | |
| cred_model = catalogue.validate_model(cred_model) | |
| except ValueError: | |
| cred_model = None | |
| fallback_model = cred_model or fallback_model | |
| if not env_base and not config_base and cred.get("base_url"): | |
| fallback_base = cred["base_url"] |
🤖 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/praisonai-code/praisonai_code/llm/env.py` around lines 207 - 223, The
fallback metadata lookup in env.py is reusing stored model/base_url data without
verifying it belongs to the same API key. Update the fallback_lookup/provider
handling in this branch so `cred["model"]` and `cred["base_url"]` are only
adopted when the stored credential key matches the active env/config API key,
and otherwise skip that stored metadata. Keep the change localized around the
fallback credential selection logic and any helper that compares credential
identity.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Ruff no longer reports the silent broad exception in env.py.
fd -p 'src/praisonai-code/praisonai_code/llm/env.py' -x ruff check {} --select S110,BLE001Repository: MervinPraison/PraisonAI
Length of output: 3606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai-code/praisonai_code/llm/env.py"
echo "== top of file =="
sed -n '1,40p' "$file"
echo
echo "== आसपास lines 210-235 =="
sed -n '210,235p' "$file"
echo
echo "== logger references =="
rg -n '\blogger\b|logging\.getLogger|import logging' "$file"Repository: MervinPraison/PraisonAI
Length of output: 2697
Log the metadata-recovery failure. Keep this best-effort path, but emit a debug log instead of swallowing Exception so credential-store issues are diagnosable.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 226-227: try-except-pass detected, consider logging the exception
(S110)
[warning] 226-226: 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/praisonai-code/praisonai_code/llm/env.py` around lines 226 - 227, The
metadata recovery path in the environment handling currently swallows all
exceptions in the credential-store lookup, which hides diagnosable failures.
Update the exception handler in the relevant metadata-recovery logic in env.py
to keep the best-effort behavior but emit a debug-level log with the caught
exception instead of using a bare pass. Use the surrounding env/credential-store
recovery flow and any existing logger in this module to preserve context and
make failures traceable.
Source: Linters/SAST tools
1 participant
Summary
llm/env.pyandllm/credentials.pyintopraisonai_codeso credential gates (is_configured,inject_credentials_into_env, endpoint resolution) work without importing thepraisonaiwrappersys.modulesredirect) so existingpraisonai.llm.*imports and test patches keep workingapp,main,run,init,model_resolver) and route Langfuse/Langextract observability through_wrapper_bridgeis_configuredimports without wrapperTest plan
pytest tests/unit/test_c5_backward_compat.py tests/unit/llm/test_env_resolver.py tests/unit/llm/test_credentials_is_configured.py tests/unit/cli/test_init_provider_guard.py— 94 passedpraisonai-agents+praisonai-codeonly):from praisonai_code.llm.credentials import is_configuredsucceedsSummary by CodeRabbit